Make position morph binding as optional in renderers + enable other attrib type morph

[noname0310]

In #16014 it is now possible to disable position morphs, but since we changed all renderers that use morph targets to unconditionally add the MORPHTARGETS_POSITION define, I tried to use those renderers with meshes that don't have position morphs and found that it caused problems.

This PR fixes the issue mentioned above and enables normal, tangent, uv, and uv2 morphing, which were present in the renderers' shader code but not working in practice. (This is an additional issue I discovered while editing the code)

This is a list of fixed renderers Each of the morph types supported by these renderers existed in the shader but were not actually working because the isReady function did not add the appropriate attributes and define:

• DepthRenderer - uv, uv2
• EffectLayer - uv, uv2
• GeometryBufferRenderer - normal, uv, uv2
• OutlineRenderer - normal, uv, uv2
• ShadowGenerator - normal, uv, uv2
• VolumetricLightScatteringPostProcess - uv, uv2

This is a list of fixed renderers that had code that did not account for the case where the Morph Target Manager does not support Position Morph:

• ComputeShaderBoundingHelper
• TransformFeedbackBoundingHelper
• MorphTargetsBlock

The following defines have been added to handle cases where the list of morph data types that the MorphTargetManager has on the texture does not match the list of morph data types required by the shader.

MORPHTARGETS_SUPPORTPOSITIONS
MORPHTARGETS_SUPPORTNORMALS
MORPHTARGETS_SUPPORTTANGENTS
MORPHTARGETS_SUPPORTUVS
MORPHTARGETS_SUPPORTUV2S

See morphTargetsVertex.fx for more details

[bjsplat]

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

[bjsplat]

Snapshot stored with reference name:
refs/pull/16024/merge

Test environment:
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16024/merge/index.html

To test a playground add it to the URL, for example:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16024/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/16024/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/16024/merge
https://gui.babylonjs.com/?snapshot=refs/pull/16024/merge
https://nme.babylonjs.com/?snapshot=refs/pull/16024/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/16024/merge#BCU1XR#0

[bjsplat]

Visualization tests for WebGPU (Experimental)

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16024/merge/testResults/webgpuplaywright/index.html

[bjsplat]

WebGL2 visualization test reporter:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16024/merge/testResults/webgl2playwright/index.html

[Popov72]

Thanks, I'd forgotten that we can have data in the morph texture that we don't use in the shader code if the user decides to disable them!

However, I don't think your code is right yet.

MorphTargetManager.supportPositions will only return true if the manager has data for the positions AND if the user enabled the use of this data. So, something like this is wrong:

if (manager.supportsPositions) {
    defines.push("#define MORPHTARGETS_SUPPORTPOSITIONS");
    defines.push("#define MORPHTARGETS_POSITION");
}

We should push MORPHTARGETS_SUPPORTPOSITIONS as soon as the manager has data for positions, not just if the user has chosen to use it. manager._supportsPositions is the property that should be tested instead :

if (manager._supportsPositions) {
    defines.push("#define MORPHTARGETS_SUPPORTPOSITIONS");
}
if (manager.supportsPositions) {
    defines.push("#define MORPHTARGETS_POSITION");
}

However, this property is private, and I think we should make the code clearer. I suggest renaming all the _supportsXXX in MorphTargetManager to _hasXXX (moreover, this will match the name of these properties in MorphTarget) and adding some hasPositions, hasNormals, etc. getters. The above code would become :

if (manager.hasPositions) {
    defines.push("#define MORPHTARGETTEXTURE_HASPOSITIONS");
}
if (manager.supportsPositions) {
    defines.push("#define MORPHTARGETS_POSITION");
}

Note that I've also changed the name of the first define, for greater clarity.

Also, #if defined(MORPHTARGETS_SUPPORTPOSITIONS) || defined(MORPHTARGETS_POSITION) can be simplified to #if defined(MORPHTARGETS_SUPPORTPOSITIONS), as MORPHTARGETS_SUPPORTPOSITIONS will always be defined if MORPHTARGETS_POSITION is defined (with the rename, it would be #if defined(MORPHTARGETTEXTURE_HASPOSITIONS)).

How does it look like?

Note: of course, these changes should also be done for the other targets (normals, tangents, etc)!

[EDIT] I forgot to comment on one thing. Some of the renderers didn't push certain morph targets because the shader code doesn't use them (uv for the depth renderer, for example). However, I guess there's no problem keeping your changes, as the corresponding shader code will be dead code and will be removed by the compiler.

[noname0310]

Thank you for your response on the weekend.

Also, #if defined(MORPHTARGETS_SUPPORTPOSITIONS) || defined(MORPHTARGETS_POSITION) can be simplified to #if defined(MORPHTARGETS_SUPPORTPOSITIONS), as MORPHTARGETS_SUPPORTPOSITIONS will always be defined if MORPHTARGETS_POSITION is defined (with the rename, it would be #if defined(MORPHTARGETTEXTURE_HASPOSITIONS))

The reason I set this condition is because of backwards compatibility.
If this PR is merged, external renderer (shader) code that doesn't take MORPHTARGETS_SUPPORTTPOSITIONS into account will not work. However, if that's not a big deal, we can change this to a simple test as you suggest.
(Honestly, given that the current implementation of morph targets doesn't support any morphing other than position morph across several renderers, and there are no reports of it, I'm guessing that no one has used morph targets that deeply yet.)

[EDIT] I forgot to comment on one thing. Some of the renderers didn't push certain morph targets because the shader code doesn't use them (uv for the depth renderer, for example). However, I guess there's no problem keeping your changes, as the corresponding shader code will be dead code and will be removed by the compiler.

The depth renderer uses UV morphs. If you look at the shader code, you can see that the alpha test uses a uv with a UV morph applied. When modifying the other renderers, I referenced the shader code to make sure that only the morph types used are added to the defines.

If there's anything else wrong, please let me know. I'm applying what you requested.

[Popov72]

Thank you for your response on the weekend.

Also, #if defined(MORPHTARGETS_SUPPORTPOSITIONS) || defined(MORPHTARGETS_POSITION) can be simplified to #if defined(MORPHTARGETS_SUPPORTPOSITIONS), as MORPHTARGETS_SUPPORTPOSITIONS will always be defined if MORPHTARGETS_POSITION is defined (with the rename, it would be #if defined(MORPHTARGETTEXTURE_HASPOSITIONS))

The reason I set this condition is because of backwards compatibility. If this PR is merged, external renderer (shader) code that doesn't take MORPHTARGETS_SUPPORTTPOSITIONS into account will not work. However, if that's not a big deal, we can change this to a simple test as you suggest. (Honestly, given that the current implementation of morph targets doesn't support any morphing other than position morph across several renderers, and there are no reports of it, I'm guessing that no one has used morph targets that deeply yet.)

Yes, I think we can keep the simplified code, and see if anyone complains that we've broken their code.

[EDIT] I forgot to comment on one thing. Some of the renderers didn't push certain morph targets because the shader code doesn't use them (uv for the depth renderer, for example). However, I guess there's no problem keeping your changes, as the corresponding shader code will be dead code and will be removed by the compiler.

The depth renderer uses UV morphs. If you look at the shader code, you can see that the alpha test uses a uv with a UV morph applied. When modifying the other renderers, I referenced the shader code to make sure that only the morph types used are added to the defines.

My bad, I even checked the code of the depth renderer before replying...

[Popov72]

We have this type of code 7 or 8 times, I think we should try to factor it.

Instead of modifying PrepareAttributesForMorphTargetsInfluencers, you could create a new PrepareDefinesAndAttributesForMorphTargets function that would be something like this (warning: not tested!):

function PrepareDefinesAndAttributesForMorphTargets(
    morphTargetManager: MorphTargetManager,
    defines: string[],
    attribs: string[],
    mesh: AbstractMesh,
    usePositionMorph = true,
    useNormalMorph = false,
    useTangentMorph = false,
    useUVMorph = false,
    useUV2Morph = false
): void {
    const numMorphInfluencers = morphTargetManager.numMaxInfluencers || morphTargetManager.numInfluencers;
    if (numMorphInfluencers <= 0) {
        return;
    }

    defines.push("#define MORPHTARGETS");

    if (morphTargetManager.hasPositions) defines.push("#define MORPHTARGETTEXTURE_HASPOSITIONS");
    if (morphTargetManager.hasNormals) defines.push("#define MORPHTARGETTEXTURE_HASNORMALS");
    if (morphTargetManager.hasTangents) defines.push("#define MORPHTARGETTEXTURE_HASTANGENTS");
    if (morphTargetManager.hasUVs) defines.push("#define MORPHTARGETTEXTURE_HASUVS");
    if (morphTargetManager.hasUV2s) defines.push("#define MORPHTARGETTEXTURE_HASUV2S");

    if (morphTargetManager.supportsPositions && usePositionMorph) defines.push("#define MORPHTARGETS_POSITION");
    if (morphTargetManager.supportsNormals && useNormalMorph) defines.push("#define MORPHTARGETS_NORMAL");
    if (morphTargetManager.supportsTangents && useTangentMorph) defines.push("#define MORPHTARGETS_TANGENT");
    if (morphTargetManager.supportsUVs && useUVMorph) defines.push("#define MORPHTARGETS_UV");
    if (morphTargetManager.supportsUV2s && useUV2Morph) defines.push("#define MORPHTARGETS_UV2");

    defines.push("#define NUM_MORPH_INFLUENCERS " + numMorphInfluencers);

    if (morphTargetManager.isUsingTextureForTargets) {
        defines.push("#define MORPHTARGETS_TEXTURE");
    }

    _TmpMorphInfluencers.NUM_MORPH_INFLUENCERS = numMorphInfluencers;
    _TmpMorphInfluencers.NORMAL = useNormalMorph;
    _TmpMorphInfluencers.TANGENT = useTangentMorph;
    _TmpMorphInfluencers.UV = useUVMorph;
    _TmpMorphInfluencers.UV2 = useUV2Morph;

    PrepareAttributesForMorphTargets(attribs, mesh, _TmpMorphInfluencers, usePositionMorph);
}

With this function, you can replace all the boilerplate code with:

if (manager) {
    PrepareDefinesAndAttributesForMorphTargets(manager, defines, attribs, mesh, ...);
}

[noname0310]

I realized during refactoring that there is no function in the materialHelper.functions file that handles string[] type defines. They all handle the case of using the MaterialDefines class.
For this reason, it seems inconsistent to add a function to this file that handles string[] define. This is actually why I didn't add the new helper function.

I think we need a refactoring to unify the way we handle the defines first, so we can reuse the same prepare code.

Anyway, I'll make the change as requested.

[Popov72]

I don't think it's a problem to add the function in materialHelper.functions, but I'll let @sebavan do the final review when he comes back (early next week).

Thanks a lot for the PR!