Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect lights direction when exported to glb 0, 1 or 2 times #7377

Closed
noalak opened this issue Dec 30, 2019 · 13 comments
Closed

Incorrect lights direction when exported to glb 0, 1 or 2 times #7377

noalak opened this issue Dec 30, 2019 · 13 comments
Assignees
Labels
bug exporters in progress
Milestone

Comments

@noalak
Copy link
Contributor

@noalak noalak commented Dec 30, 2019

Primary playground is #KX53VK#30 testing right handed export to glb.
All bugs could be related, so I grouped them here.

Black triangles

Bug repro: https://www.babylonjs-playground.com/#9DXCHP#1
Expected result: Triangles should be colored
Current result: When scene is exported and imported once (not twice), the triangles are black (unlighted).
Comments:
The light direction is back to front (0,0,-1).
When imported into the sandbox, the triangles are visible due to a default ambient light. The directional light is still back to front and the gizmo toggle has no effect on light direction. Those behaviors in the sandbox are as expected.

Light direction

Bug repro: https://www.babylonjs-playground.com/#KX53VK#30
Expected result: The light should be oriented as initially front to back (0,0,1).
Current result: When scene is exported and imported twice (unmodified playground), the light direction is reversed: oriented back to front (0,0,-1).
Comments:
The gizmo toggle has no effect on light direction (unlike other 2 bugs above).
Triangles are lighted correctly, despite light direction being reversed.

Linked light dummies

Bug repro: https://www.babylonjs-playground.com/#KX53VK#30
Expected result: Each Light dummy should be independant from each other
Current result: the 2 Light dummies, parent of the actual directional light, are someway linked together. Transform edition of one dummy through Inspector impacts the other. Comparing world matrix using the console.

I'm off the office until Monday 6th.
Adding @Jaskar for any question meanwhile.

@deltakosh
Copy link
Contributor

@deltakosh deltakosh commented Dec 31, 2019

Adding @thomlucc to track them

@noalak @Jaskar can you guys work on it?

@deltakosh deltakosh added this to the 4.1 milestone Dec 31, 2019
@Drigax
Copy link
Contributor

@Drigax Drigax commented Jan 9, 2020

Linking
BabylonJS/Exporters#696

As my test process touches on the light gizmo not aligning with the light's actual orientation

@thomlucc thomlucc assigned Drigax and unassigned Jaskar and noalak Jan 9, 2020
@Drigax
Copy link
Contributor

@Drigax Drigax commented Jan 10, 2020

Few initial takeaways:
I'll start by figuring out why the gizmo is improperly oriented and/or modifies the light orientation on activation in a Right-Handed coordinate space. This seems key to being able to quickly visually debug the other elements.

I'd assume that we're just improperly inverting the light z scale when exporting the scene. This seems similar to our double root issues via #6349 where we're converting our already converted light transform, but I'll have to actually dig in and debug to confirm this.

@noalak for the linked light dummy issue, you linked to the original PG:
https://www.babylonjs-playground.com/#KX53VK#30
Is this correct? This scene appears to only contain a single light. Also I'm not sure what "light dummy" means in this context, can you please clarify?

Thanks

@noalak
Copy link
Contributor Author

@noalak noalak commented Jan 10, 2020

2020-01-10_10h19_42

The light dummies are the light meshes parenting the actual light. They are used as Transform nodes when importing a glTF light.
If you edit in the Inspector the rotation of the node selected in the image above, and then select its parent, the displayed rotation is affected as well.
World matrices of light dummies seem right (the parent world matrix is unchanged). The bug is probably on the Inspector refresh.

@noalak
Copy link
Contributor Author

@noalak noalak commented Jan 22, 2020

Do you have all the informations you need to fix those bugs ?

@Drigax
Copy link
Contributor

@Drigax Drigax commented Feb 6, 2020

Thanks @noalak, I think I have all I need to make progress on this.

I'm moving the gizmo related issue to a separate bug: #7603 as It's not really related to these issues, just a very annoying occurance when trying to reproduce this.

@Drigax Drigax added the in progress label Feb 6, 2020
@Drigax
Copy link
Contributor

@Drigax Drigax commented Feb 6, 2020

In investigating the light export issue, I am noticing some strange behavior of the lights when we use a right handed coordinate system:

I am using #KX53VK#32 to reproduce this (using my #7603 fix). When I use a right handed coordinate system:

image

I have a directional light in scene configured so that it should illuminate my triangle mesh, however it renders as black:
image

This does not reproduce in a left handed system.

Some investigation with @bghgary indicates that this change:
https://github.com/BabylonJS/Babylon.js/pull/6712/files/8dcdbf7386243e7acdededa6d8b8ecde436d1b4c#r313200449

is related to some of the issues that I am observing in addressing the original export issue, but I'm not totally sure if this is the sole cause of the triangles being shaded black in our test scene.

@sebavan, @CedricGuillemet , would you be able to help advise?

@CedricGuillemet
Copy link
Contributor

@CedricGuillemet CedricGuillemet commented Feb 6, 2020

The triangle is black because the face winding doesn't correspond to the normal.
I think this happens because you have 2 swaps in your PG.

  • one with the positions depending on RH/LH
  • one with the ComputeNormals

The 1st swap will make the normal flipped (because of the positions), then you flip again the normal with the ComputeNormals call.
As the positions correspond to what you want, you don't need to flip the normals a second time.

https://www.babylonjs-playground.com/#KX53VK#33

@Drigax
Copy link
Contributor

@Drigax Drigax commented Feb 6, 2020

Thanks @CedricGuillemet,
The test scene appears to work now, but I'm very concerned about the orientation of the vertex normals in our right handed system now (see https://www.babylonjs-playground.com/#KX53VK#34) :

image

If we use the vertex normal gizmo, it shows the normals as pointing backwards from the plane? This doesn't seem right either, the generated plane in scene seems generated with proper normal orientation and winding order, but is also lit backwards:
image
image

I'm starting to suspect that we're lighting our scene wrong in right handed mode...

@Drigax
Copy link
Contributor

@Drigax Drigax commented Feb 6, 2020

After talking with @CedricGuillemet , he pointed me to this thread which prompted the original change to the vLightSpecular direction:
https://forum.babylonjs.com/t/solved-unexpected-light-gizmo-on-right-hand-system/5043/15

This looks very similar to the change I just addressed via #7603, looks like we conflated a lighting error with the gizmo error. I believe that it's for the best to fix this, as we can reproduce this regression by modifying this testing scene:
https://www.babylonjs-playground.com/#0YYQ3N#0
image
by changing the scene handedness before import:
image

Which causes our poor cat to be eerily lit. Undoing commit https://github.com/BabylonJS/Babylon.js/pull/6712/files/8dcdbf7386243e7acdededa6d8b8ecde436d1b4c#r313200449 appears to fix this regression in combination with the previously mentioned gizmo fix:
image

@Drigax
Copy link
Contributor

@Drigax Drigax commented Feb 6, 2020

Now this causes our original test scene: https://www.babylonjs-playground.com/#KX53VK#30
To fail on validation. I believe that we originally did not have proper light orientation, as the directional light instantiated in scene was facing (0,0,1) regardless of handedness. I think this one slid through the cracks and our original test scene was not properly constructed.

I will update this testing scene to work correctly as a part of the light direction reverting PR; the lights should face our triangles regardless of scene handedness. The unit tests will be updated to reference:
https://www.babylonjs-playground.com/#KX53VK#35 (Left handed)
https://www.babylonjs-playground.com/#KX53VK#36 (right handed)

@bghgary
Copy link
Contributor

@bghgary bghgary commented Feb 6, 2020

It would be good to verify that the double root issue is also fixed once ready.

@Drigax
Copy link
Contributor

@Drigax Drigax commented Feb 14, 2020

Regarding the light orientation issue, there are multiple issues happening here:

For one, our lights aren't being properly exported from a right handed scene, they are 180degY off, as the exported node should be oriented such that the light shines towards -Z:

image

This appears to be caused by using the same calculation for the light's yaw angle given the direction, a right handed system should invert the input direction Z axis to align with the coordinate system:

const yaw = -Math.atan2(localAxis.z, localAxis.x) + Math.PI / 2;

In addition, the light direction is toggled, due to a duplication of light dummy nodes. On import, we create a dummy node oriented such that the light has a direction of (0,0,-1) regardless of scene handedness. On export, we preserve that node, and export that dummy node with the light parented to that node as a new node containing the KHR_lights_punctual extension. This node then duplicated on the next import, causing the light to flip direction.

image

In order to fix this issue, I had to make some alterations to the _applyExtension contract (and affected methods and interfaces that use this contract)

Originally, there was no capability to use or return a null value from an export extension handler method, my changes intended to expand the contract to allow for an extension to "void" an exported node via returning null. In this case we don't want to duplicate the existing exported node, after modifying its parent. From there, all that's needed is to reorient the exported grandparent node of the light to encapsulate the light.

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

No branches or pull requests

7 participants