Skip to content

Conversation

castano
Copy link

@castano castano commented Aug 20, 2025

Normal maps are often compressed using the BC5 and EAC_RG formats. This is currently not supported by three.js as the shaders expect the sampled normal map values to contain RGB components representing the packed XYZ coordinate.

To address this issue I propose we add a projectZ attribute to the NormalMapNode object. When this attribute is true, the generated shader code computes the Z component of the normal by projecting the XY components over the hemisphere.

Texture loaders are expected to set the projectZ attribute of the texture, which in turns activates the code generation in the corresponding NormalMapNode.

At the moment there are no texture loaders doing this, but this is something that the existing KTX and KTX2 loaders could benefit from. Additionally, I'd like to use this feature in texture loaders that use the spark.js encoders.

This contribution is funded by Ludicon

When the projectZ attribute is true this computes the Z component of the normal by projecting the XY components over the hemisphere.
Copy link

github-actions bot commented Aug 20, 2025

📦 Bundle size

Full ESM build, minified and gzipped.

Before After Diff
WebGL 338.91
79.11
338.96
79.12
+48 B
+13 B
WebGPU 578.16
159.25
578.8
159.53
+640 B
+275 B
WebGPU Nodes 576.76
159.01
577.4
159.28
+640 B
+273 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Before After Diff
WebGL 470.78
113.9
470.78
113.9
+0 B
+0 B
WebGPU 648.69
175.28
649.16
175.45
+466 B
+170 B
WebGPU Nodes 602.79
164.41
603.25
164.57
+466 B
+167 B

@@ -235,6 +235,12 @@ class MaterialNode extends Node {
node = normalMap( this.getTexture( 'normal' ), this.getCache( 'normalScale', 'vec2' ) );
node.normalMapType = material.normalMapType;

if ( material.normalMap.projectZ ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not rely on monkey-patched code here.

How about you introduce the property in the CompressedTexture class? Or is it also relevant for Texture?

Alternatively, move the property into the userData field of normalMap for now.

Copy link
Collaborator

@Mugen87 Mugen87 Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When thinking about this: Why not checking for specific RG texture formats here and only then set the node property to true? In this way we need no additional texture property and it would not be necessary to update loaders.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is most useful in CompressedTexture, but in my particular application I'm using this with ExternalTexture and it can also be useful with uncompressed textures, to reduce their size by storing only the RG components.

I agree this should be done in some other way. A potential problem with this PR is that the attribute won't be cloned with the texture and that may cause issues. Adding it to the userData field as you propose would solve that problem.

I'm not sure enabling the Z reconstruction is the right thing to do for all RG texture formats. RG textures can be used for other purposes in addition to normal maps (heat distortion maps, 2d vector fields, chrominance values, etc). It may not cause any harm to just compute the Z component this way, but this won't extend to ASTC normal maps if we ever want to support that.

Copy link
Author

@castano castano Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with the userData approach. Let me know if that looks good to you.

Copy link
Collaborator

@Mugen87 Mugen87 Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure enabling the Z reconstruction is the right thing to do for all RG texture formats.

My idea was to only do this for specific map types. So for a normal map, a format like bc5-rg-snorm (or SIGNED_RED_GREEN_RGTC2_Format) must hold packed RG data, otherwise it would be unusable for a normal map. This is something that could be implemented in the node classes so loaders don't have to worry about this detail.

But it's true a separate property allows more flexibility than deriving from texture formats.

It would be interesting to hear the opinion of others about this PR. Would you be okay if we label this PR as a "Draft PR" for now? I have the feeling we need to discuss the API a bit more in the team/community before making a decision.

Copy link
Author

@castano castano Aug 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, now that I think about it, it could make sense to do it based on the format. This happens inside the NormalMapNode, so I think it's safe to assume the texture being sampled is indeed a normal map.

The explicit unpack mode allows supporting ASTC LA normal maps or DXT5nm and in these cases it's not possible to infer the packing mode from the format alone.

I don't see an option to convert this into a Draft PR, but(Found the link) I'd be happy to hear other people's feedback.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The explicit unpack mode allows supporting ASTC LA normal maps or DXT5nm and in these cases it's not possible to infer the packing mode from the format alone.

@donmccurdy I'll ping you directly since you have done so much work in context of texture compression.

Similar to Texture.colorSpace, what do you think about introducing a new property Texture.packing that describes the packing strategy of a texture. The default would be NoPacking (as a pendant to NoColorSpace).

Packing strategies usually apply on certain texture types so probably most of the new constants for Texture.packing would be quite specific (like RGNormalPacking or GANormalPacking). But that is a similar situation like with Texture.colorSpace where certain constants only apply to color textures.

So...what do you think about this approach? Do you have maybe alternatives in mind?

Copy link
Collaborator

@donmccurdy donmccurdy Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't be opposed to adding a Texture#packing property, I think @bhouston had proposed something similar once? A few other thoughts:

  1. If we see texture.format = THREE.RGFormat (or similar) assigned to a normal map slot, possibly that is enough in this case?

  2. In the newer TSL/NodeMaterial APIs with WebGPURenderer, is a property on THREE.Texture sure to be accessible? Or could the Texture instance have been wrapped in other nodes, obscuring the underlying Texture? If so, we could handle this by a new node to handle the unpacking, or a material property similar to material.normalMapType, instead.

@Mugen87 Mugen87 added this to the r181 milestone Aug 25, 2025
@castano castano marked this pull request as draft August 26, 2025 06:39
}

setup( { material } ) {

const { normalMapType, scaleNode } = this;
const { normalMapType, scaleNode, unpackNormal } = this;

let normalMap = this.node.mul( 2.0 ).sub( 1.0 );
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use the colorToDirection function in Packing.js

That said, in the case of normal maps it may make more sense to use:

let normalMap = this.node.mul( 2.0 ).sub( 254.0 / 255.0 );

As that prevents lighting discontinuities on flat normals along uv seams where the tangents do not match exactly.

@castano
Copy link
Author

castano commented Aug 27, 2025

To add a bit more motivation, here's a detail from SciFiHelmet showing the normals encoded as RGB using BC7, RG using BC5 (this PR) and RGB uncompressed:

image image image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants