RFR: 8234920: Add SpotLight to the selection of 3D light types

[Kevin Rushforth]

On Tue, 8 Dec 2020 22:49:01 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 165:
>> 
>>> 163:      * The angle is clamped between 0 and 180.
>>> 164:      *
>>> 165:      * @defaultValue 90
>> 
>> Should this be 180 be default? I don't have a strong opinion on this.
>
> 180 will make it look like a point light, which is probably not what the users want out of the box. The value is arbitrary at the end of the day, so I don't really mind.

A default of 90 seems fine.

>> modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 194:
>> 
>>> 192:      * values are allowed, but give unrealistic lighting.
>>> 193:      *
>>> 194:      * @defaultValue 1
>> 
>> Maybe it should default to 0? I don't have a strong opinion on this either.
>> 
>> I do think that it should say that values < 0 are clamped to 0.
>
> I think that having a dropoff as a default is closer to what users will want. 0 will just give a flat circle (thought there is decreased intensity with the angle to the surface).
> 
> Is there a good reason to limit it? Nothing changes if we allow <0 values. I can remove the documentation for it, but in terms of implementation I don't see the benefit.

I think a default of 1 is fine then. Also, I don't have the same concern over negative falloff values as I do for the inner and outer angles.

>> modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 59:
>> 
>>> 57:  * effect by setting {@code innerAngle} to be larger than {@code outerAngle} and lowering {@code maxRange}.
>>> 58:  * <p>
>>> 59:  * <img src="doc-files/spotlightRing.png" alt="Image of the ring light effect">
>> 
>> I don't see the value in supporting this type of effect. It would potentially constrain the implementation and would complicate the description of the lighting equation.
>> 
>> Without this complication, the equation as described in the earlier paragraph is effectively:
>> 
>> if (a < innerAngle) {
>>     I = 1
>> } else if (a > outerAngle) {
>>     I = 0
>> } else {
>>     I = pow((cos(a) - cos(outer)) / (cos(inner) - cos(outer)), falloff)
>> }
>> In order to support the ring effect, you would need define the equation differently (i.e., it doesn't just naturally derive from the above equation). In any case, it seems best to just remove this, and document that the valid range of `outerAngle` is `[innerAngle, 180]`. We could clamp on usage as we do in a few other areas. Similarly, we probably want to document that `falloff` should be non-negative.
>
> Similar to what I said about the falloff, I can remove the documentation, but I don't see a compelling reason to artificially limit the values. The equation is as described in the docs. If someone puts in values outside of physically realistic ranges they will still get whatever the equation spits out. Do you think that there would be a too-large surprise factor?
> 
> One thing to note is that the user can do the clamping themselves (or we can do it later if we don't commit in the docs), but if we clamp, the user can't unclamp.

The main issue I have is that the specified equation matches the proposed implementation only for values that satisfy the range constraints: `0 <= innerAngle <= outerAngle <= 180`.

The reason for this is that the shader implementation compares cosines not angles, and clamps the result of the division rather than testing whether the angle is < inner or > outer. These are very reasonable implementation choices.

We have a few options that I can see:

1. List the expected range, and specify that values outside the expected range are clamped on usage.
2. List the expected range, and specify that the results are undefined for values outside the expected range.
3. Describe the equation that is actually happening.
4. Implement what the equation actually specifies, even for out-of-range values. This would constrain the implementation and likely require extra if tests in the shader.

Option 4 makes no sense to me. I don't care for 3, since it is less easy to explain and understand, and it precludes other implementations that are equivalent for in-range values but might behave differently for out-of-range values.

So that leaves option 1 or 2 as workable options. I prefer option 1, although I'll admit we aren't consistent on this point: we let the implementation do whatever it will do with out of range color values for PhongMaterial or Lights.

-------------

PR: https://git.openjdk.java.net/jfx/pull/334