On Tue, 8 Dec 2020 22:49:01 GMT, Nir Lisker <nlis...@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
