On Wed, 9 Dec 2020 01:06:45 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> 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. > > I see. > > I also dislike 3 and 4. I prefer 2, but if you are convinced that 1 is better > I'll go with it. I'm OK with option 2. So that would mean removing any mention of the "ring" effect, and documenting that the valid ranges are: innerAngle: [0, 180] outerAngle: [innerAngle, 180] But we wouldn't clamp them or otherwise enforce it. Instead we could add a sentence that the results are undefined (or maybe "unpredictable"?) if the angles are outside that range. ------------- PR: https://git.openjdk.java.net/jfx/pull/334
