SpotLightHelper: Fix offset when adjusting the scene / parent position

[gkjohnson]

Fixed #27437

Description

Previously SpotLightHelper was just copying the target light's matrixWorld into its local matrix field resulting in offsets when the parent was not at 0, 0, 0. Now the target light transform is correctly transformed into the local frame that the helper exists in and it's no longer required to add the helpers to the root of the scene.

This is for another PR but I'm wondering if "update" can just be removed and effectively called in "updateMatrixWorld"?

[github-actions]

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev	Filesize PR	Diff
676.2 kB (167.9 kB)	676.4 kB (168 kB)	+193 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev	Filesize PR	Diff
456.8 kB (110.7 kB)	456.8 kB (110.7 kB)	+0 B

[Mugen87]

I understand this works but I'm a bit unhappy about the resulting complexity in updateMatrixWorld(). If the helper would have to be a child of the light, I assume things could be much easier.

[gkjohnson]

If the helper would have to be a child of the light, I assume things could be much easier.

The requirement to add helpers to the root scene has confused me countless times in the past. Asking users to add a helper to a specific parent so that it works betrays all expectation around the transform hierarchy works. This would be a significantly worse user experience for the helpers.

I understand this works but I'm a bit unhappy about the resulting complexity in updateMatrixWorld().

I don't agree that it's so complicated. These are matrix transformations that are used throughout the project. But either way this is a transformation that should be used across all helpers - BoxHelper, CameraHelper, DirectionalLightHelper, etc should all implement this the same way. If this is added then we should add a super class that shares an updateMatrixWorld implementation among all child helper classes.

Can you elaborate on what you feel is complex?

[Mugen87]

Can you elaborate on what you feel is complex?

The need to overwrite updateMatrixWorld() to begin with. But also the code itself is not something I would label as trivial.

My point is I favor an assumption like "spot light helper must be child of the light" if that makes the overall implementation easier.

[gkjohnson]

The need to overwrite updateMatrixWorld() to begin with.

There are quite a few other classes that also overwrite updateMatrixWorld including other Helpers. I would argue it's more user friendly to do this with the Helper-classes, anyway, since it means "update" doesn't need to be called. But either way the code here can be moved to the "update" function if this is an issue.

But also the code itself is not something I would label as trivial.

The transformation happening is no different than the one required for transforming an object into the camera frame. It's fundamental to rendering graphics.

[Mugen87]

I'm okay with merging this new version. As long as we keep the current helper policy we should make sure to fix #27437 and that's what the PR does.

[gkjohnson]

As long as we keep the current helper policy

What's the expectation for helpers? Some of them update themselves in updateMatrixWorld (Box3Helper, SkeletonHelper, PlaneHelper) while others require manually calling an update function (CameraHelper, DirectionalLightHelper, and the rest). Should their APIs be normalized?

[Mugen87]

If we want to refactor the world update matrix logic at some point, it would be good if updateMatrixWorld() and updateMatrix() are not overwritten in subclasses of Object3D.

[WestLangley]

Why are you doing this -- instead of retaining this.matrixAutoUpdate = false ?

[gkjohnson]

I suppose I could do that. It would probably be best to se matrixWorldAutoUpdate to false but tbh the matrix / matrixWorld update behavior is fairly confusing to me, now. For example the Object3D.lookAt function will call updateWorldMatrix which will automatically update the objects matrixWorld even if matrixWorldAutoUpdate is false.

It's not clear to me when they do and don't apply so I fallback to keeping all the values in sync which I think is more clear, anyway.

[WestLangley]

I'd avoid unnecessarily calling matrix.decompose() for this helper. JMO, of course.

[WestLangley]

Is this matrix necessarily current?

[gkjohnson]

I suppose that's true - I can call this.parent.updateMatrixWorld but it has the same caveats as mentioned in #27487 (comment) in that it will undermine the users parent.matrixWorldAutoUpdate setting if it's set to false.