SpotLight map affected by distance updates

[kalegd]

Description

I recently noticed that when updating the distance of a spotlight to 0, the texture shined on a mesh is affected by whether the distance to the mesh prior was larger or smaller than the previous spotlight.distance value. When the prior spotlight.distance value was smaller than the distance to the mesh, the map is not reflected on the object. If it was larger, the map is still there as expected. Please see the jsfiddle below for an example

jsfiddle: https://jsfiddle.net/3rj7u8n9/99/

I'm assuming this is not expected behavior and could be a bug? I've tested this on both my Macbook Pro with Chrome and on iOS Safari and the behavior is the same on both

Reproduction steps

1. Create a spotlight with a map set and any mesh to shine the light on
2. Set the spotlight distance to some value less than the distance to the mesh
3. Set the spotlight distance to 0 (the map is no longer shown on the mesh

Code

// Please see jsfiddle for example

Live example

https://jsfiddle.net/3rj7u8n9/99/

Screenshots

No response

Version

r159

Device

Desktop, Mobile

Browser

Chrome, Safari

OS

MacOS, iOS

[WestLangley]

Thank you for your excellent example.

The offending line in the three.js source is:

https://github.com/mrdoob/three.js/blob/5b96dbc07f1218d43f3cf0b3ac3bc6c19c34e3f5/src/lights/SpotLightShadow.js#L23

In the OP's fiddle, spotLight.distance is changed periodically, resulting in a change to spotLight.shadow.camera.far.

The following table shows the sequence of changes in the fiddle:

dist	far
0	500
10	10
0	10
2	2
0	2
10	10

Updated fiddle with a temporary hack to prevent the artifact: https://jsfiddle.net/679d0f23/

[kalegd]

Thank you for pinpointing the source of the issue so fast! My first thought was so use a default value of 500 instead of camera.far, but looks like that would be a regression of this pull request https://github.com/mrdoob/three.js/pull/11164 and not allow users to update the spotLight.shadow.camera.far field directly anymore

I'll try and spend some time thinking about a solution this weekend if not tonight and share my findings

[kalegd]

I can think of two potential solutions to this issue

Adding a setDistance method to SpotLight

class SpotLight {
    ...
    setDistance( distance ) {
        this.distance = distance;
        if ( !distance )
            this.shadow.camera.far = Math.max( this.shadow.camera.far, 500 );
    }
    ...
}

Tracking prior light.distance and camera.far values in SpotLightShadow

class SpotLightShadow {
    ...
    constructor() {
        ...
        this._lastLightDistance = 0;
        this._lastCameraFar = 500;
        ...
    }

    updateMatrices( light ) {
        ...
        const far = light.distance
            || ( this._lastLightDistance && camera.far === this._lastCameraFar
                ? Math.max( camera.far, 500 )
                : camera.far );

        this._lastLightDistance = light.distance;
        this._lastCameraFar = camera.far;
        ...
    }
    ...
}

Additional Note: The default minimum value of 500 for camera.far is in alignment with default value provided in the SpotLightShadow constructor

I strongly prefer the first option, but present the second as well in case you all want to handle these edge cases via directly updating spotlight.distance instead of adding a setter method for distance for whatever reason

Let me know your thoughts and I'll be more than happy to throw up a pull request

[Mugen87]

I would expect this behavior:

dist	far
0	camera.far
10	10
0	camera.far
2	2
0	camera.far
10	10
1000	1000
0	camera.far

Meaning the code always uses SpotLight.distance and only falls back to far if the distance value is 0. Note that 500 is just the default value of far and should not be hardcoded in updateMatrices().

I prefer the change in SpotLightShadow since this does not require any changes on the user side to fix the issue. Besides, I'm not feeling so well when light classes manipulate their shadow property. None of the lights does this so far. It seems more understandable to encapsulate this particular logic in SpotLightShadow.

[kalegd]

I had a feeling there might be a preference for the second, and the reasoning makes sense to me. I just like having getters and setters haha

With respect to using camera.far instead of 500, I'm assuming you mean to track the last camera.far that was set by the user? The current logic is already using the last camera.far value which gets set by far in SpotLightShadow and causes the bug

Tracking updates to camera.far occurring outside of updateMatrices would give us something like

class SpotLightShadow {
    ...
    constructor() {
        ...
        this._lastCameraFar = 500;
        this._lastUpdatedCameraFar = 500;
        ...
    }

    updateMatrices( light ) {
        ...
        if ( this._lastCameraFar != camera.far ) {
            this._lastUpdatedCameraFar = camera.far;
        }
        const far = light.distance || this._lastUpdatedCameraFar;
        ...
        this._lastCameraFar = camera.far;
    }
    ...
}

[Mugen87]

I realize the status quo is quite confusing so how about we introduce a new property SpotLight.shadowLimit that limits the distance of shadows when the range of the spot light is infinite (that is when its distance property is 0). With this approach, we can do just this:

const far = light.distance || light.shadowLimit;

Meaning the shadow camera's far value is always coupled to the light's distance except when it has no limit. In this case, we need to control a shadow limit for proper shadow map generation and a separate property seems to simplify this.

We could also move the property to SpotLightShadow which is probably even better.

[kalegd]

That feels much easier to read. +1 on it being a part of the SpotLightShadow property

This would affect anyone currently adjusting the SpotLightShadow's camera.far property manually. They'd need to adjust the shadowLimit property instead now. Just calling that out in case it needs to be in the wiki's Migration Guide

I'll get a pull request up shortly