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

Helpers: Ensure light helpers are not frame-late. #21427

Merged
merged 3 commits into from
Oct 31, 2022
Merged

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Mar 7, 2021

Fixed #14801.

Description

Ensures the world matrices of lights and targets are up-to-date when evaluated in light helpers.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 9, 2021

Just want to clarify that this issue can't be fixed by overwriting updateMatrixWorld() in the helpers. Doing this does not guarantee that the world matrix of the reference object (the light) is up-to-date.

@mrdoob
Copy link
Owner

mrdoob commented Mar 9, 2021

Just want to clarify that this issue can't be fixed by overwriting updateMatrixWorld() in the helpers. Doing this does not guarantee that the world matrix of the reference object (the light) is up-to-date.

Hmm, in what case the reference object's wold matrix won't be up to date by the time the helper's updateMatrixWorld() runs?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 10, 2021

Consider this hierarchy:

Scene - Helper
      - Light

And assume scene.updateMatrixWorld() is called. When the helper's world matrix is updated first, it will use an outdated version of the light's world matrix.

And even when the helpers call light.updateMatrixWorld(), this method will probably not update a light's world matrix correctly since it does not update the matrices of the ancestor 3D objects.

@acu192
Copy link
Contributor

acu192 commented Apr 8, 2021

This is EXACTLY the PR I was planning to send in when I wrote issue #21608. Nice work!

Well, one difference actually: I would do the same change for PointLightHelper, address this line:

this.light.updateMatrixWorld();

@acu192
Copy link
Contributor

acu192 commented Apr 8, 2021

PS: This is a big problem being addressed here. It's easy to create a light helper that shows you the wrong thing, in which case it does the opposite of helping (it hurts you).

The main fix here is calling this.light.target.updateWorldMatrix(...) in the same places that you call this.light.updateWorldMatrix(...). Consistency in updating the light's matrix and target's matrix is kind of important. :) I ran into this bug in my project and it seems (based on the other issued filed) others have as well.

Granted, the long-term solution is to remove targets from lights (and use the light's quaternion instead), but until that solution is in-place, we need to address the current bug. This PR fixes the current bug and does not add any breaking changes (at least, not that I can imagine).

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 8, 2021

I've updated PointLightHelper as requested!

@acu192
Copy link
Contributor

acu192 commented Apr 19, 2021

@mrdoob Do you think we get this in the r128 release?

@WestLangley WestLangley added this to the r130 milestone Jun 12, 2021
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jun 21, 2021

BTW: This is what users do in order to desperately make light helpers work:

const lightHelper = new THREE.DirectionalLightHelper(light, 10);
lightHelper.name = 'lightHelper';
scene.add(lightHelper);
lightHelper.parent.updateMatrixWorld();
lightHelper.update();

From: https://discourse.threejs.org/t/directionallight-shadow-camera-helper-pointing-in-the-wrong-direction/27392

With this PR, the user could just add the helper and things work out-of-the-box. Would love to see this get merged in r130 😇 .

@mrdoob mrdoob modified the milestones: r130, r131 Jun 30, 2021
@mrdoob mrdoob modified the milestones: r131, r132 Jul 28, 2021
@mrdoob mrdoob modified the milestones: r132, r133 Aug 26, 2021
@mrdoob mrdoob modified the milestones: r133, r134 Sep 30, 2021
@mrdoob mrdoob modified the milestones: r134, r135 Oct 28, 2021
@mrdoob mrdoob modified the milestones: r135, r136 Nov 26, 2021
@acu192
Copy link
Contributor

acu192 commented Dec 18, 2021

@mrdoob Anything we can do to help you feel comfortable with this?

Excited to get it in r136. 😉

@mrdoob mrdoob modified the milestones: r136, r137 Dec 24, 2021
@mrdoob mrdoob removed this from the r137 milestone Jan 26, 2022
@mrdoob mrdoob added this to the r138 milestone Jan 26, 2022
@mrdoob mrdoob modified the milestones: r138, r139 Feb 23, 2022
@mrdoob mrdoob modified the milestones: r139, r140 Mar 24, 2022
@mrdoob mrdoob modified the milestones: r140, r141 Apr 30, 2022
@mrdoob mrdoob modified the milestones: r141, r142 May 26, 2022
@mrdoob mrdoob modified the milestones: r142, r143 Jun 29, 2022
@mrdoob mrdoob modified the milestones: r143, r144 Jul 28, 2022
@mrdoob mrdoob modified the milestones: r144, r145 Aug 31, 2022
@mrdoob mrdoob modified the milestones: r145, r146 Sep 29, 2022
@mrdoob mrdoob modified the milestones: r146, r147 Oct 27, 2022
@Mugen87 Mugen87 merged commit 39e5d3e into mrdoob:dev Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DirectionalLightHelper is a frame late at best
4 participants