Open leonbloy opened 1 week ago
The PR affects the velocity shader in https://threejs.org/examples/webgl_materials_channels
This is also the only module where the mentioned packing/unpacking routines are used.
@bhouston Since these functions were added in #23784, do you mind having a look at this issue?
Live example with changes proposed in this post: (panning is enabled in this example) https://raw.githack.com/westlangley/three.js/dev-pack_depth/examples/webgl_materials_channels.html
Master branch: (panning disabled) https://threejs.org/examples/webgl_materials_channels.html
@Rabbid76 I've seen at stackoverflow that you have quite some experience in this area. In your post about depth packing you essentially implement the point differently that is raised by the OP. The order of the pack factors is reversed. Should we update our RGBA packing routines?
If I'm not mistaken, packDepthToRGBA has a more fundamental problem:
packDepthToRGBA(1.0) = packDepthToRGBA(0.0) = vec4(0, 0, 0, 0)
This is good stuff. These encoders/decoders have always been problematic.
BTW I implemented a glsl unit test system in Threeify. Would be amazing to get something similar to that in Three.js at some point:
https://github.com/bhouston/threeify/blob/main/packages/core/src/shaders/math/unitIntervalPacking.test.glsl https://github.com/bhouston/threeify/blob/main/packages/core/src/shaders/microgeometry/normalPacking.test.glsl
Basically It would run these shaders and the asset would write 0 or 1 into a texture which I would then read back and check:
https://github.com/bhouston/threeify/blob/main/examples/src/units/glsl/index.ts
Formally proposed as a new feature here: https://github.com/mrdoob/three.js/issues/28708
I'm not sure if it's possible to do something safe, because the packing/unpacking needs to know exactly how the mapping float-uint8 is done internally, round( v * 255.0)
or floor(v * 255.999...)
or whatever. And, if I'm not mistaken, this is not mandated
@leonbloy:
I'm not sure if it's possible to do something safe, because the packing/unpacking needs to know exactly how the mapping float-uint8 is done internally, round( v 255.0) or floor(v 255.999...) or whatever. And, if I'm not mistaken, this is not mandated
You could use a similar idea to the unit tests, but it would be a quick test that is run initially on load to profile the GPU / drivers, to determine the packing details and then use that information in order to pick which packing method to use in the full shaders.
My fix:
Here I made a fiddle , which tests in javascript the packing/unpacking for all cases, with alternative float-uint8 conversions (the common one, fquant255_a , and the alternative fquant255_b) It seems it works ok, the relative error (error/ 256^components) does not exceed the expected values. It works slightly better for the common quantization) If asked, i can provide the GLSL versions
I think it would be good to have a PR for this. In this way, we can easier evaluate the suggested changes.
All four points of your suggested fix sound good to me.
make similar (but slightly) different functions for 3 "resolutions" : RGBA (4 bytes), RGB (3 bytes) RG (2 bytes)
Even if we don't need the RGB routines, it can't hurt to have working versions in the repository.
Here is an implementation of mine, with a sandbox to test it
https://codesandbox.io/p/sandbox/threejs-depth-packing3-5wklnl
I think the RGB and RG variants should be added as alternatives to the MeshDepthMaterial.depthPacking constant https://threejs.org/docs/#api/en/materials/MeshDepthMaterial.depthPacking
And I think that the RGB should be preferred over RGBA. With 24 bits (and even 16 bits) of precision we already cover the vast majority of practical uses. Also, considering that the float simple precision is 24 bits, in real life, when using RGBA the least significant channel (be it R or A) will be just noise (or zero).
I have updated the live examples posted above to include the latest changes proposed by @leonbloy.
I did a few round-trip tests with the new routines and everything seems to work as expected.
@WestLangley Since you have already updated the channels example, would you file these changes as a PR? The new constants THREE.RGDepthPacking
and THREE.RGADepthPacking
sound good to me.
Description
packing.glsl.js
includes several packDepth / unpackDepth functions.packDepthToRGBA
packs a float in [0,1] range to a vec4 in the same range,unpackRGBAToDepth
does the reversepackDepthToRG
unpackRGToDepth
do the same in low resolution (vec2)I see two problems here:
(minor) By some strange design decision, packDepthToRGBA uses the fourth (A) component for the most significant part, red for the least significant part. This does not matter if one is only interested in converting to-from a vec4, but seems inconvenient for visual inspection - and it's also cumbersome if the alpha component gets lost in the pipeline. It would seem more natural to me to pack in reverse order.
(major)
packDepthToRG
unpackRGToDepth
are wrong, because they ignore the above, they take the RG components ofpackDepthToRGBA
(the least significant), setting the most significant part to zero.I attach an example with a fixed version (for the second issue, I suspect that the first one could have more impact to change)
Reproduction steps
float f2 = unpackRGToDepth( packDepthToRG( f ))
does not return (not even approximately) the original valueYou can see in the attached live example that the composing works for the original high-res packDepthToRGBA - unpackRGBAToDepth and for my fixed versions packDepthToRG2 unpackRGToDepth2
Code
Live example
Screenshots
No response
Version
r165
Device
Desktop
Browser
Chrome
OS
Windows