Add overrideMaterialFillMode property to Mesh

[MiikaH]

As discussed in forum thread here: https://forum.babylonjs.com/t/add-mesh-overridematerialfillmode/39475/

It seems to work fine for playground /#WX0F9G. The model has 2 meshes, each with bunch of instances, both share same material. The playground example changes overrideMaterialFillMode on the other mesh only.

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

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/13688/merge

Test environment:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13688/merge/index.html

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

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13688/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/13688/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/13688/merge
https://gui.babylonjs.com/?snapshot=refs/pull/13688/merge
https://nme.babylonjs.com/?snapshot=refs/pull/13688/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/13688/merge#BCU1XR#0

[Popov72]

Thanks for the PR!

However, I wonder why you put getFillMode on the subMesh, when it is only related to the material and mesh?

To mimic what we have for alpha blending, with material.needAlphaBlending when we want the setting for the material only and material.needAlphaBlendingForMesh when we want to apply it to a specific mesh, I'd opt for Material.getFillModeForMesh in the implementation.

cc @sebavan to get his opinion.

Regarding:

it("No more than 144 clean properties on a mesh", () => {

I don't think you can change the 128 limit, I seem to remember that it's a limit in Chrome where things get much slower because the object is not stored/managed internally in the same way when the number of properties reaches that limit...

Instead of adding overrideMaterialFillMode as a direct property of Mesh, you should instead add it inside the _InternalMeshDataInfo structure: this structure was added to overcome the 128 property limitation.

[MiikaH]

However, I wonder why you put getFillMode on the subMesh, when it is only related to the material and mesh?

Because SubMesh seemed to be the only common factor between all use cases where fillMode is used.

Say, take a look at:

Babylon.js/packages/dev/core/src/Meshes/abstractMesh.ts

Line 1795 in e787ede

!!material && subMesh.getFillMode(material) === Constants.MATERIAL_TriangleStripDrawMode

Right now it is pretty short and clear: subMesh.getFillMode(material).
If that was changed to a material method like you suggested then it would be: material.getFillModeForMesh(subMesh.getRenderingMesh()) which is not only much longer but also can create confusion as to which mesh to use.

Instead of adding overrideMaterialFillMode as a direct property of Mesh, you should instead add it inside the _InternalMeshDataInfo structure: this structure was added to overcome the 128 property limitation.

Interesting. I'll take a look if I can figure that out.

[Popov72]

What bothers me is that conceptually, fillMode is not related to subMesh, so having the method in the subMesh class doesn't seem right...

Let's see what @sebavan thinks.

[MiikaH]

Yeah I understand. I was spending quite bit of time trying to think how to do it myself.

A plain function would require adding imports everywhere. Mesh or Material submethod requires calling SubMesh.getRenderingMesh() anyway in many cases so it kind of is tied to SubMesh anyway.

Anyway, how I ended up thinking of it is just a @internal helper function. Its not a getter or property tied to SubMesh itself. Just a helper function to get the required fillMode when dealing with SubMeshes. (Like all code parts that used fillMode were)

[MiikaH]

Pushed a fix for not using _InternalMeshDataInfo and restored the 128 test

[sebavan]

As @Popov72 I would as well prefer the fill mode on the mesh rather than the submesh.

I would also remove the scene parameter from it as it is only used once in scene. so we prevent extra checks in all the other cases.

Looking more closely the override seems to only be needed in 4 places:

1. in _bind
2. in _processRendering
3. collide
4. and in the depth peeling

For the first 2 If it all belongs to the mesh like _bind and _processRendering does, it could be computed inside ? I wonder if it would make things simpler

[MiikaH]

For the first 2 If it all belongs to the mesh like _bind and _processRendering does, it could be computed inside ? I wonder if it would make things simpler

I already considered that but unfortunately I don't think that would work. If applied inside then:

• It would be impossible for any part in code to override the fillMode for _bind or _processRendering, because the mesh override would be applied on top
• And also it would then override the scene level option

[MiikaH]

I would also remove the scene parameter from it as it is only used once in scene. so we prevent extra checks in all the other cases.

Scene override is used by effectLayer._renderSubMesh() and mesh.render(), so yeah I suppose that is kind of redundant to centralize. Could make more sense if additional render paths that use it are added in future.

I'll go ahead and move the helper function to Mesh and remove that scene arument then.

[MiikaH]

Ok, did those changes now.

Looks like it gained couple extra .getRenderingMesh() calls. Personally I'm not sure if that is better if it always and only ever uses subMesh renderingMesh for the override anyway..

[sebavan]

@MiikaH We gave this one more thoughts with @Popov72 earlier today and we think this could do it #13708 so instead of adding tons of feedback here I drafted it to discuss the difference before merging.

It is all your code just remapped a tiny bit without the part not related to the rendering (as we figured those info are really geometry based)

What do you think ?