packDepthToRG seems broken

[leonbloy]

packDepthToRG seems broken

[WestLangley]

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

[Mugen87]

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.

[leonbloy]

My fix:

• force the value when v=1.0
• correct the last element of UNPACK_FACTORS
• change the order RGBA -> R is the most significant
• make similar (but slightly) different functions for 3 "resolutions" : RGBA (4 bytes), RGB (3 bytes) RG (2 bytes)

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

https://jsfiddle.net/hjgonzalez984/2z39qw8d/41/

[leonbloy]

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).

[WestLangley]

I have updated the live examples posted above to include the latest changes proposed by @leonbloy.

[Mugen87]

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?

[Mugen87]

@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?

[leonbloy]

If I'm not mistaken, packDepthToRGBA has a more fundamental problem:

packDepthToRGBA(1.0) = packDepthToRGBA(0.0) = vec4(0, 0, 0, 0)

[bhouston]

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: #28708

[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

[bhouston]

@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.

[Mugen87]

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.