-
-
Notifications
You must be signed in to change notification settings - Fork 35.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Examples: Add UltraHDRLoader #28825
Examples: Add UltraHDRLoader #28825
Conversation
To sum up, the idea of this PR is to change the way the engine loads Ultra HDR images. Right now, the example webgl_loader_texture_hdrjpg relies on the third-party loader In this case, I would probably remove |
Adjust files.json Add 2K hdr.jpg texture
d35baa1
to
fdc4cd7
Compare
Yes, the only regression compared to
Done. Gainmap converter is linked in UltraHDRLoader example, credit to MONOGRID also given 🙇♂️ |
Unfortunately, I see a performance regression with the new loader (tested with Chrome 126.0.6478.127 and MacOS 14.5). I use the following to measure times: const loader = new HDRJPGLoader( renderer );
console.time( 'hdr' );
const hdrJpg = await loader.loadAsync( 'textures/equirectangular/spruit_sunrise_4k.hdr.jpg' );
console.timeEnd( 'hdr' ); And in your example: loader = new UltraHDRLoader();
loader.setDataType( THREE.FloatType );
console.time( 'hdr' );
const texture = await loader.loadAsync( 'textures/equirectangular/spruit_sunrise_4k.hdr.jpg' );
console.timeEnd( 'hdr' ); With With |
True - was unrealistic on my side. Gotten way too overoptimistic and measured quickly and incorrectly - esp. that Trimmed
Could you confirm the numbers from third column in a spare moment? Considering Since it's a loader, not a real-time feature, before I'd dive too deep into overoptimisations:
(Updated PR description ✅) |
Adjust for DeepScan
845e66e
to
1de6b05
Compare
It is important that certain components are developed in third-party repositories and not everything in this one. Otherwise the maintenance burdens is too high and we focus too less on the core features of the engine. There must be a good reason for adding a component and tbh I don't see it in this case. I would rather prefer continue using a third-party loader as adding one to the repository. How the third-party loader is then developed and how complex it is internally does not really matter to use as long as the API is consistent to other loaders. Looking at the numbers, TBH, not much is speaking for an addition of this loader. Even if the performance would be on pair I see no reason for moving Ultra HDR loader to the main repository as long as https://github.com/MONOGRID/gainmap-js is actively maintained. The final decision is up to @mrdoob though so I defer to him at this point. |
Understandable 🫡 Just to step back to my main motivation behind including a simpler version of Waiting for @mrdoob 🙌 |
100% agree with this! I remember trying to push for a simpler cpu-only version but I can't find the conversation now. Maybe it was a private conversation with @elalish... |
While you're at it, would you like to give it a go at doing a |
Okay, that is a good point. If Hopefully the parsing performance can be improved in the future so the delay gets a bit less noticeable when loading a 4K texture. |
I was just trying to replace
I used gainmap-creator to convert venice_sunset_1k.hdr to UltraHDR (0.95 quality). |
@mrdoob - I'll take a look and see why that may be happening - could be something with the recent optimisations or the SRB-to-Linear table. Will make a separate PR. @kristiker - I'll check how much that'll speed up,
Yes, I had code for it before doing
To match |
const linearHDRValue = Math.min( | ||
Math.max( | ||
this._srgbToLinear( hdrValue ), | ||
0 | ||
), | ||
65504 | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just trying to replace RGBELoader with UltraHDRLoader in webgl_tonemapping and I'm seeing some data loss...
I'm wondering if the sRGB-to-Linear conversion should be moved to affect only the SDR value on line 539? Have not tested this yet but will take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The color grading issue was with the channel index not being applied to the gainmap image (gainmap loop used only the red channel because of that 🙇♂️ )
As for SRGB-to-Linear - according to spec, that's indeed correct. Only the SDR image should be converted and gainmap image is assumed to be linear right away. The reason for moving it to the last step is in Canvas API (here) - currently it allows only rendering colors in SRGB & Display-P3, which I think accidentally converts the gainmap from LinearSRGB to SRGB (either this step or the loading the image via <img>
) 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the gainmap is neither sRGB nor Linear-sRGB, it is non-color data. When the gainmap is drawn to the Canvas API, I'd expect the Canvas API to either (a) observe no color space and draw the gainmap without conversion, or (b) assume the gainmap is already sRGB and (again) draw the gainmap without conversion. In either case, calling getImageData
with colorSpace: 'srgb'
is the right choice, as it again avoids any conversions, which would inherently be unwanted on non-color data.
Which is not to say that something in the loading process couldn't be applying an unwanted conversion ... the web platform does not really understand either Linear-sRGB or non-color identifiers well ... but I think we can avoid that if so.
Very glad to see UltraHDR coming into the fold! I still think the GPU is the right way to go, but that may entail some refactoring of how loading is done. Still, especially with WebGPU coming along with compute shaders, I think we'd do well to have a renderer available to offload parallel compute to in any function, not just the actual renderer. |
And I'm all for reducing external dependencies, but I'm not sure where that 3MB came from - when I added gainmap-js to model-viewer, it only added a few kb. |
I think they mean the size of |
I believe the encoder is 99% of that size; the decoder is quite small. |
Yes, that's what I meant. |
Yes, the decoder is small but they come in a single package (incl. that 3MB of WASM.) It's not a problem for final builds, but to add UltraHDR to drei for ex., both the decoder+WASM encoder would need to be added as a single peerDependency.
GPU is 100% the way to go, and can be added to the current UltraHDRLoader quite easily - but, unless I'm mistaken, the amount of |
Yeah, ideally I think we should always have a single context, or at least make that the golden path. The question is how to architect for that? One option is to pass a context to the loader, but that is perhaps less clean. Another option, like PMREM, is to do all the processing lazily and internally, so the renderer will just build the final texture when needed. |
Related issue: N/A
Description
Adds
UltraHDRLoader
to examples, dropping the external dependencies / WASM modules.👉 Demo & comparison with gainmap-js and RGBELoader
Inspired by MONOGRID/gainmap-js, read through UltraHDR docs and the format turns out to be a bit too trivial to justify 3MB dependency currently required to load the files in three.js.
This implementation follows the specs and is compatible with gainmap-creator HDRs, without any external dependencies (based almost entirely on this method from the original Cpp implementation.)
PMREMGenerator
s orWebGLRenderer
s are created, loader follows the style ofRGBELoader
, with a SRGB_TO_LINEAR in-memory table added for a 4x speed-up when loading 4K textures (3x for 2K, 2x for 1K.)Speed comparison
gainmap-js