The STL loader flips the faces

[YusakuNo1]

The current code loads STL files with faces flipped for the following combinations (RH for useRightHandedSystem, NO_ALTER for DO_NOT_ALTER_FILE_COORDINATES):

	NO_ALTER-true	NO_ALTER-false
RH-true	good	bad
RH-false	bad	good

The correct combination for whether flip the face indices is like the following table, which means, we should only detect DO_NOT_ALTER_FILE_COORDINATES but not useRightHandedSystem:

	NO_ALTER-true	NO_ALTER-false
RH-true	flip	no-flip
RH-false	flip	no-flip

[azure-pipelines]

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.

[azure-pipelines]

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

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

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

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

Links to test babylon tools with this snapshot:

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

[sebavan]

I do not understand how this will flip the faces in right handed mode after this PR which is needed ?

Not the space I am the most familiar with but I guess rightHanded should be used somewhere ?

@bghgary any thoughts ?

[bghgary]

I have the same thought. We have to take rightHanded into account somewhere?

[YusakuNo1]

+1 I'm wondering as well, the result came from my experiments, but I don't fully understand. Let me dive deeper and update back to this thread.

[sebavan]

Moving to draft in the mean time

[YusakuNo1]

I think I found the issue. First, I don't think we need to update index for right handed system because it's a "runtime" behavior, doesn't need to update data. Instead, it's calculated during runtime, switch back face culling from CCW to CW. For example, one of these "runtime" approaches are: the setter of useRightHandedSystem marks all the materials dirty and then they will be updated later, and the materials will be updated with flag useRightHandedSystem; second (which is obvious) we need to update indexes if Y & Z axis are swapped during loading time for STL file. It's the reason why the PR only need to consider DO_NOT_ALTER_FILE_COORDINATES.

This is the playground demonstrates: when I set the index as CCW for LH, it works for both LH and RH, even though in RH, it's CW.
https://playground.babylonjs.com/#FQ74IH#6

[sebavan]

Makes kinda sense :-)

@bghgary, you can try
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13317/merge/index.html#1DY3CF#4
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13317/merge/index.html#1DY3CF#5
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13317/merge/index.html#1DY3CF#6
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13317/merge/index.html#1DY3CF#7

For all the combinations DO_NOT_ALTER_FILE_COORDINATES and useRightHandedSystem with the stl loader.

[sebavan]

And this is still OK #12957 Thanks a lot for the fix !!!

[bghgary]

I see. The STL loader doesn't need to check useRightHandedSystem because STL uses the default material which already accounts for it. I think we should probably add some tests for these cases, but it doesn't have to be part of this PR.