packDepthToRG seems broken

[leonbloy]

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 reverse

packDepthToRG unpackRGToDepth do the same in low resolution (vec2)

I see two problems here:

1. (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.

2. (major) packDepthToRG unpackRGToDepth are wrong, because they ignore the above, they take the RG components of packDepthToRGBA (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

1. Check that the composed transformation float f2 = unpackRGToDepth( packDepthToRG( f )) does not return (not even approximately) the original value

You 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

// normalized coordinates, (0,0) is the bottom left
in vec2 uv;
// resulting fragment color, you may name it whatever you like
out vec4 fragColor;

// --- from https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/packing.glsl.js ------

const float PackUpscale = 256. / 255.; // fraction -> 0..1 (including 1)
const float UnpackDownscale = 255. / 256.; // 0..1 -> fraction (excluding 1)

const vec3 PackFactors = vec3( 256. * 256. * 256., 256. * 256., 256. );
const vec4 UnpackFactors = UnpackDownscale / vec4( PackFactors, 1. );

const float ShiftRight8 = 1. / 256.;

vec4 packDepthToRGBA( const in float v ) {
	vec4 r = vec4( fract( v * PackFactors ), v );
	r.yzw -= r.xyz * ShiftRight8; // tidy overflow
	return r * PackUpscale;
}

float unpackRGBAToDepth( const in vec4 v ) {
	return dot( v, UnpackFactors );
}

vec2 packDepthToRG( in highp float v ) {
	return packDepthToRGBA( v ).yx;
}

float unpackRGToDepth( const in highp vec2 v ) {
	return unpackRGBAToDepth( vec4( v.xy, 0.0, 0.0 ) );
}

// ---------- my own "fixed" versions

vec2 packDepthToRG2( in highp float v ) {
	return packDepthToRGBA( v ).zw;
}

float unpackRGToDepth2( const in highp vec2 v ) {
	return unpackRGBAToDepth( vec4(0.0, 0.0,v) );
}

// should produce a uniform gradient - comment/uncomment fragColor = lines to try
void main() {
	vec2 st = uv;
    float r = unpackRGBAToDepth(packDepthToRGBA(st.x));
    float g = st.y;
    bool plain = fract(st.x * 5.0 + st.y) > 0.5; // bands
    // fragColor = plain ? vec4(r,g,0.0,1.0) : vec4( unpackRGBAToDepth(packDepthToRGBA(r)), g, 0.0, 1.0); // this works
    fragColor = plain ? vec4(r,g,0.0,1.0) : vec4( unpackRGToDepth(packDepthToRG(r)), g, 0.0, 1.0); // this does not work
    // fragColor = plain ? vec4(r,g,0.0,1.0) : vec4( unpackRGToDepth2(packDepthToRG2(r)), g, 0.0, 1.0); // FIXED 
}

Live example

• glsl.app

Screenshots

No response

Version

r165

Device

Desktop

Browser

Chrome

OS

Windows

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

[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]

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

[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/

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

[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]

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.