Fix for Inverted Y Component on Gltf Normal Map Loading #2120

codex128 · 2023-10-17T22:05:14Z

This PR fixes #2098 by setting the normal type to -1 instead of 1 during gltf import. Since gltf goes by OpenGL's normal map convention, and PBRLighting.j3md defines 1=OpenGL and -1=DirectX, something is clearly wrong here.

See this forum post for more info.

stephengold · 2023-10-18T03:28:37Z

I guess the documentation in PBRLighting.j3md is incorrect.

Changing the semantics of the "NormalType" material parameter would've been easy in 2014. Changing it now would be painful. I propose changing the PBRLighting documentation instead. The change could even be included in this PR, since there's a clear connection between the jme3-plugins defect and the documentation error.

riccardobl · 2023-10-18T11:18:26Z

The problem is in MikktspaceTangentGenerator, test this

package jme3test.model;

import com.jme3.app.*;
import com.jme3.math.ColorRGBA;
import com.jme3.math.Vector3f;
import com.jme3.renderer.queue.RenderQueue;
import com.jme3.scene.*;
import com.jme3.scene.plugins.gltf.GltfModelKey;
import com.jme3.system.AppSettings;
import com.jme3.util.mikktspace.MikktspaceTangentGenerator;

public class TestGltfNormal extends SimpleApplication {
    Node probeNode;

    public static void main(String[] args) {
        AppSettings sett = new AppSettings(true);
        sett.setWidth(1024);
        sett.setHeight(768);
        TestGltfNormal app = new TestGltfNormal();
        app.setSettings(sett);
        app.start();
    }

    @Override
    public void simpleInitApp() {
        rootNode.setShadowMode(RenderQueue.ShadowMode.CastAndReceive);
        probeNode = (Node) assetManager.loadModel("Scenes/defaultProbe.j3o");
        rootNode.attachChild(probeNode);

        setPauseOnLostFocus(false);

        flyCam.setEnabled(false);
        viewPort.setBackgroundColor(new ColorRGBA().setAsSrgb(0.2f, 0.2f, 0.2f, 1.0f));

        loadModel("jme3test/gltf/NormalTangentMirrorTest.glb", new Vector3f(0, 0, 0), 3);

    }

    private void loadModel(String path, Vector3f offset, float scale) {
        GltfModelKey k = new GltfModelKey(path);
        Spatial s = assetManager.loadModel(k);
        s.scale(scale);
        s.move(offset);
        // MikktspaceTangentGenerator.generate(s);
        probeNode.attachChild(s);
    }

}

GLB file: https://github.com/KhronosGroup/glTF-Sample-Models/blob/master/2.0/NormalTangentMirrorTest/glTF-Binary/NormalTangentMirrorTest.glb
NormalTangentMirrorTest has pregenerated tangents and it renders correctly.
Now enable this line // MikktspaceTangentGenerator.generate(s); that regenerate the tangents with our generator, and now the normals are flipped again.

This is caused by the sign of the bitangent (line 282 in MikktspaceTangentGenerator)

                mikkTSpace.setTSpaceBasic(tang, pTSpace.orient == true ? 1.0f : (-1.0f), f, i);

replacing it with

                mikkTSpace.setTSpaceBasic(tang, pTSpace.orient == true ? -1.0f : 1.0f, f, i);

Solves the issue.
However i don't know if there is an underlying issues that is causing pTSpace.orient to be true when in fact it should be false or if this is just a mismatch in handedness with the generator logic and our expected result.

I suspect this is affecting a lot more than the PBR shader, for example if we look into SPLighting that doesn't have the normal type parameter, it says

      //Note the -2.0 and -1.0. We invert the green channel of the normal map, 
      //as it's compliant with normal maps generated with blender.
      //see http://hub.jmonkeyengine.org/forum/topic/parallax-mapping-fundamental-bug/#post-256898
      //for more explanation.
      vec3 normal = normalize((normalHeight.xyz * vec3(2.0,-2.0,2.0) - vec3(1.0,-1.0,1.0)));

but blender uses OpenGL Y+ format, see:

source
The link in the comments is from the old forum, searching it in google points to https://hub.jmonkeyengine.org/t/parallax-mapping-fundamental-bug/28352

I suspect there was a misunderstanding, because the original jme tangent generator also had mismatched handedness, so in this case flipping the flipped sign results in correct representation of OpenGL Y+ normals and this ported to the mikkstspace generator that suffers the same issue, however what happens to imported models with Y+ normals and pregenerated tangents that use SP lighting? I supposed they will have flipped normals like in the gltf test.

So, maybe we should attempt an engine wide fix for all the shaders and tangents generators and restandardize the default format to be Y+ for all the engine code.
This would probably result in some breaking change for the cases where the developer somehow eyeballed the right normal format for the specific combination of tangent/shader/normaltype , however i suspect the vast majority of imported models suffer from this bug

riccardobl · 2023-10-18T11:42:46Z

If someone has the time to convert the GLTF test into different formats, including both pregenerated and non-pregenerated tangents, and SP lighting, we could determine the extent of this bug.

codex128 · 2023-10-19T00:13:52Z

Ok, I'll take a crack at it.

codex128 · 2023-10-29T20:31:52Z

Finally got around to it... sorry for the delay!

I've tested GLTF, J3O, and OBJ formats, with both pregenerated and non-pregenerated tangents, with each material configuration (packed pbr, runtime pbr, runtime sp), with runtime tangent generators on and off.

J3O was tested when converted from both GLTF and OBJ. Results are not shown, because J3O mimicked its source format on all tests.

GLTF

Packed PBRLighting Material

No Pre-Generated Tangents

no runtime generator:       FAIL
TangentBinormalGenerator:   FAIL
MikktspaceTangentGenerator: FAIL

With Pre-Generated Tangents

no runtime generator:       pass
TangentBinormalGenerator:   FAIL
MikktspaceTangentGenerator: FAIL

PBRLighting Material Created at Runtime

No Pre-Generated Tangents

no runtime generator:       pass
TangentBinormalGenerator:   pass
MikktspaceTangentGenerator: pass

With Pre-Generated Tangents

no runtime generator:       FAIL
TangentBinormalGenerator:   pass
MikktspaceTangentGenerator: pass

Lighting MultiPass Material Created at Runtime

No Pre-Generated Tangents

no runtime generator:       pass
TangentBinormalGenerator:   pass
MikktspaceTangentGenerator: pass

With Pre-Generated tangents

no runtime generator:       FAIL
TangentBinormalGenerator:   pass
MikktspaceTangentGenerator: pass

Lighting SinglePass Material Created at Runtime

No Pre-Generated Tangents

no runtime generator:       pass
TangentBinormalGenerator:   pass
MikktspaceTangentGenerator: pass

With Pre-Generated tangents

no runtime generator:       FAIL
TangentBinormalGenerator:   pass
MikktspaceTangentGenerator: pass

OBJ (no pre-generated tangents available)

PBRLighting Material Created at Runtime

no runtime generator:       no shading
TangentBinormalGenerator:   FAIL
MikktspaceTangentGenerator: FAIL

Lighting MultiPass Material Created at Runtime

no runtime generator:       no shading
TangentBinormalGenerator:   FAIL
MikktspaceTangentGenerator: FAIL

Lighting SinglePass Material Created at Runtime

no runtime generator:       shaded, but does not update to camera angle
TangentBinormalGenerator:   FAIL
MikktspaceTangentGenerator: FAIL

riccardobl · 2023-11-03T16:38:35Z

When you create the PBR material at runtime, do you set opengl format?

SPLighting Material Created at Runtime

No Pre-Generated Tangents

no runtime generator: pass
TangentBinormalGenerator: pass
MikktspaceTangentGenerator: pass

With Pre-Generated tangents

no runtime generator: FAIL
TangentBinormalGenerator: pass
MikktspaceTangentGenerator: pass

I think this shows that SP Lighting requires directx normals, what do you think?
Can you show the code?

codex128 · 2023-11-03T19:58:51Z

smh, I've made a mistake. That SPLighting data is actually for Lighting.j3md multipass. I've updated the above post to include single pass data (coincidentally, it matches the multipass data).

When you create the PBR material at runtime, do you set opengl format?

I did minimal setup for materials created at runtime, so no. I've pushed the test class, so you can see how I set it up.

I think this shows that SP Lighting requires directx normals, what do you think?

Hmm, that seems likely.

riccardobl · 2023-11-05T18:01:15Z

Thank you, i've looked into your examples, and made some considerations and some tests of my own.

My current understanding of the situation:

PBR, single-pass (SP) lighting, and multi-pass (MP) lighting all work correctly with Y+ normals internally.
The PBR shader supports both Y- and Y+ inputs by converting Y- to Y+ internally.
The SP and MP lighting shaders always convert Y- to Y+ since they lack a normal type parameter, making them compatible only with Y- (DirectX format) normal maps.
The bitangents generated by the tangent generators have the wrong sign. This essentially flips the normals sampled from the normal map. As a result, shaders that require DirectX normals (e.g., lighting) will now require OpenGL normals, and shaders that require OpenGL normals will need DirectX normals.

This issue shows different symptoms depending on the mix of normal types, textures, and bitangents. It can be tricky to spot because some combinations actually work correctly, which is likely why it went unnoticed.

Anyway, here's a summary:

SHADER	Bitangent Source	NormalType Param	Required Type*	REPORT
PBR Lighting	In model	OpenGL	OpenGL	OK
PBR Lighting	In model	DirectX	DirectX	OK
PBR Lighting	JME Generator	OpenGL	DirectX	FLIPPED BY BAD TBN
PBR Lighting	JME Generator	DirectX	OpenGL	FLIPPED BY BAD TBN
Lighting (SP)	In model	N/A	DirectX	OK
Lighting (MP)	In model	N/A	DirectX	OK
Lighting (SP)	JME Generator	N/A	OpenGL	FLIPPED BY BAD TBN
Lighting (MP)	JME Generator	N/A	OpenGL	FLIPPED BY BAD TBN

*Actual type of the input NormalMap required to have correct results

Please, anyone interested, take a look at my findings and let me know if I've missed anything.

Here's my plan for the patch:

Correct the sign issue in the newer MikktspaceTangentGenerator. As for the legacy tangent generator, it is probably better to leave as is because it's essentially deprecated at this point.
Add the NormalType parameter to SP/MP Lighting, with the default setting being DirectX (the expected type up to now).

This change might create a minor backward incompatibility. SP and MP lighting shaders have an implicit normal type, which might have misled developers using these shaders along with the JME tangent generator: they might have thought the shader required an OpenGL-type normal map because a (correct) DirectX normal map would appear flipped due to the bug mentioned earlier.

However, this change addresses a more significant issue: developers using the PBR shader and relying on the specified normal type would have encountered incorrect normal maps if they were using one of the JME tangent generators and this is likely to go mostly unnoticed, especially with complex textures.

riccardobl · 2023-11-05T20:13:13Z

Patch here: #2140
I've included a test that swap material every 5 seconds to test all the affected shaders.

It is left to check if the tangents imported from other formats (eg. ogre) have the correct sign or if they need to be flipped by the loader.

stephengold · 2023-11-05T20:19:00Z

I don't have a deep understanding of tangent conventions, but everything I've seen so far is consistent with MikktspaceTangentGenerator getting the sign backward, so I support that portion of the change.

initial problem fixed

747d6ad

stephengold added the defect Something that is supposed to work, but doesn't. Less severe than a "bug" label Oct 18, 2023

riccardobl added bug Something that is supposed to work, but doesn't. More severe than a "defect". and removed defect Something that is supposed to work, but doesn't. Less severe than a "bug" labels Oct 18, 2023

reverted normal type

cc27887

codex128 marked this pull request as draft October 27, 2023 11:17

performed tests

05e8972

riccardobl mentioned this pull request Nov 5, 2023

Fix sign of tbn generator that caused normal map flipping in some situations #2140

Merged

stephengold added this to the Future Release milestone Nov 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for Inverted Y Component on Gltf Normal Map Loading #2120

Fix for Inverted Y Component on Gltf Normal Map Loading #2120

codex128 commented Oct 17, 2023 •

edited

Loading

stephengold commented Oct 18, 2023

riccardobl commented Oct 18, 2023 •

edited

Loading

riccardobl commented Oct 18, 2023

codex128 commented Oct 19, 2023

codex128 commented Oct 29, 2023 •

edited

Loading

riccardobl commented Nov 3, 2023 •

edited

Loading

codex128 commented Nov 3, 2023

riccardobl commented Nov 5, 2023

riccardobl commented Nov 5, 2023 •

edited

Loading

stephengold commented Nov 5, 2023

Fix for Inverted Y Component on Gltf Normal Map Loading #2120

Are you sure you want to change the base?

Fix for Inverted Y Component on Gltf Normal Map Loading #2120

Conversation

codex128 commented Oct 17, 2023 • edited Loading

stephengold commented Oct 18, 2023

riccardobl commented Oct 18, 2023 • edited Loading

riccardobl commented Oct 18, 2023

codex128 commented Oct 19, 2023

codex128 commented Oct 29, 2023 • edited Loading

GLTF

Packed PBRLighting Material

PBRLighting Material Created at Runtime

Lighting MultiPass Material Created at Runtime

Lighting SinglePass Material Created at Runtime

OBJ (no pre-generated tangents available)

PBRLighting Material Created at Runtime

Lighting MultiPass Material Created at Runtime

Lighting SinglePass Material Created at Runtime

riccardobl commented Nov 3, 2023 • edited Loading

codex128 commented Nov 3, 2023

riccardobl commented Nov 5, 2023

riccardobl commented Nov 5, 2023 • edited Loading

stephengold commented Nov 5, 2023

codex128 commented Oct 17, 2023 •

edited

Loading

riccardobl commented Oct 18, 2023 •

edited

Loading

codex128 commented Oct 29, 2023 •

edited

Loading

riccardobl commented Nov 3, 2023 •

edited

Loading

riccardobl commented Nov 5, 2023 •

edited

Loading