Three.js: PointLightHelper and SpotLightHelper positioning problem

[gospodnetic]

Description of the problem

When added to the light, PointLightHelper and SpotLightHelper have wrong position.
Their position relative to the light is the same as the position of the light in world coordinates, which causes displacement.

You can see it in the fiddle for PointLightHelper, the box is of size (2,2,2) and the light is displaced by 1 along the x-axis. Correct behavior would be that the origin of the marker coincides with box side, however, it is double the distance.
https://jsfiddle.net/c14r0qmt/8/

I've found that the problem comes from copying the world matrix of the original light:

this.matrix = light.matrixWorld;
this.matrixAutoUpdate = false;

https://github.com/mrdoob/three.js/blob/dev/src/helpers/PointLightHelper.js#L14-L15
https://github.com/mrdoob/three.js/blob/dev/src/helpers/SpotLightHelper.js#L18-L19

Removing those lines solves the problem

Three.js version

• [x] r120

Browser

• [x] Chrome

[Mugen87]

The implementation assumes that both helpers are always added to the scene and not as a child to their reference object.

https://jsfiddle.net/4uw91hdk/

[gospodnetic]

Got it, I was led by the reimplementation of RectAreaLightHelper which does not require that.

Thinking about it, does it make sense to add the helper separately though? IMHO, adding it to the light and not having to care about it separately afterwards makes more sense.

[Mugen87]

The management of helpers in three.js is not consistent and sometimes confusing as this issue proves. I personally think it's way more intuitive if helper objects are attached to their reference objects and not added to the scene.

I'm not sure if this topic was ever discussed at GitHub but this is a good opportunity to do so.

[WestLangley]

This topic _has_ been discussed, and the convention from the early days of three.js has been helpers must be added a child of the scene.

The exception, now, is RectAreaLightHelper. See #14935.

To make light helpers internally-consistent, see #14658, the primary objective of which is to remove targets from lights.

EDIT: To clarify, if lights were governed by their quaternion, then we could add all light helpers as a child of the light.

[Mugen87]

This topic has been discussed

Any chances to share the respective GitHub issue? Besides, only because it was discussed once it does not mean the right decision was made.

[WestLangley]

3470 is from 2013. Maybe it will be helpful in understanding the history of this.

[mrdoob]

I have a suggestion.

I think we can override updateMatrixWorld in helpers so it just copies the matrixWorld of the target object.

That way it would not matter where in the scenegraph the user places the helper.

Helpers would behave as expected and users won't have to think about this.

@WestLangley what do you think?

[WestLangley]

As a test, I got your idea to work with RectAreaLighHelper.

I had to use onBeforeRender(). Consequently, calling update() in the render loop was no longer required. (Do you recall the reason we defined update() instead of onBeforeRender()?)

[mrdoob]

Do you recall the reason we defined update() instead of onBeforeRender()?

I think onBeforeRender() was not implemented until many years later 👴

[DefinitelyMaybe]

I'd be keen to help out with this.

[Mugen87]

BTW: If Object3D.matrix should be private at some day, this pattern should be avoided:

this.matrix = light.matrixWorld;

I mean changing the value of Object3D.matrix with the assignment operator. It's not good coding style anway.

I think it would be better if Object3D.matrix and Object3D.matrixWorld are part of this code block (and are at least not writable).

Object.defineProperties( this, {
    position: {
        configurable: true,
        enumerable: true,
        value: position
    },
    rotation: {
        configurable: true,
        enumerable: true,
        value: rotation
    },
    quaternion: {
        configurable: true,
        enumerable: true,
        value: quaternion
    },
    scale: {
        configurable: true,
        enumerable: true,
        value: scale
    },
    modelViewMatrix: {
        value: new Matrix4()
    },
    normalMatrix: {
        value: new Matrix3()
    }
} );