On Tue, 24 Nov 2020 21:56:02 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Added a SpotLight only to the D3D pipeline currently. >> >> ### API >> >> 1. Added `SpotLight` as a subclass of `LightBase`. However, it could also be >> a subclass of `PointLight` as it's a point light with direction and extra >> factors. I saw that `scenario.effect.light.SpotLight` extends its respective >> `PointLight`, but it's not a perfect analogy. In the end, I think it's a >> questions of whether `PointLight` will be expanded in a way which doesn't >> not suit `SpotLight`, and I tend to think that the answer is no. >> >> 2. The inner and outer angles are the "diameter angles" as shown >> [here](https://docs.microsoft.com/en-us/windows/win32/direct3d9/light-typeshttps://docs.microsoft.com/en-us/windows/win32/direct3d9/light-types). >> I, personally, find it more intuitive that these are the "radius angles", >> so half these angles, as used in the spotlight factor formula. Do you think >> I can change this or do you prefer the current definition of the angles? >> >> CSR will be created when the API review is finished. >> >> ### Implementation >> >> I've gotten advice from a graphics engineer to treat point lights as spot >> lights with a 360 degrees coverage, which simplifies the shader a lot and >> removes branching over the light type. This is covered anyway by a possible >> optimization in the pixel shader, which I've noted in an inline comment, >> that skips the spotlight computation if `falloff` is 0 (this is the case for >> non-spotlights). >> >> ### Performance >> >> Testing 3 point lights in order to compare with the [pre-patch >> performance](https://github.com/openjdk/jfx/pull/43#issuecomment-600632114): >> >> Without the falloff == 0 branching: >> sphere 1000 subdivisions: 120 >> mesh 5000: 9.5 >> mesh 200: 111.5 >> >> With the falloff == 0 branching: >> sphere 1000 subdivisions: 120 >> mesh 5000: 9.3 >> mesh 200: 112.5 >> >> Ubuntu 20: >> With the patch: >> Mesh 200: 60 fps >> Mesh 5000: 15 fps >> Sphere 1000: 60 fps >> >> Without the patch (master branch) >> Mesh 200: 60 fps >> Mesh 5000: 16.3 fps >> Sphere 1000: 60 fps >> >> So no major changes. I will repeat these tests to make sure there was no >> mistake. > > 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. > 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. > 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. ------------- PR: https://git.openjdk.java.net/jfx/pull/334
