On Mon, 24 May 2021 05:08:13 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> Added a SpotLight only to the D3D pipeline currently. >> >> ### API discussion points >> >> - [X] 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. >> >> - [X] 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? >> >> - [ ] The current implementation uses an ad-hoc direction property (using a >> `Point3D`). It crossed my mind that we could use the rotation transforms of >> the node to control the direction instead, just like we use the >> translation/layout of the node to get the position (there is an internal >> Affine3D transform for lights, not sure why `AmbientLight` needs it). >> Wouldn't that make more sense? When I rotate the light I would expect to see >> a change in direction. >> >> ### Implementation discussion points >> >> - [ ] I've gotten advice from a graphics engineer to treat point lights as >> spot lights with a 360 degrees coverage, which simplifies a few places. We >> can still try to optimize for a point light by looking at the light >> parameters: `falloff = 0` and `outerAngle = 180`. These possible >> optimization exist in `ES2PhongShader.java` and `D3DMeshView.cc`, and in the >> pixel/fragment shaders in the form of 3 different ways to compute the >> spotlight factor (the `computeLightN` methods). We need to check which of >> these give the best results. >> >> ## Performance >> >> Testing 3 point lights and comparing this branch with `master` using a 1000 >> division sphere, 200 meshes, and 5000 meshes. >> Using an AMD RX 470 4GB GPU. >> >> In this branch, there is a possible CPU optimization for checking the light >> type and using precalculated values (in `D3DMeshView.cc` for d3d and >> `ES2PhongShader.java` for opengl). On the GPU, I tried 3 ways of computing >> the spotlight factor contributions (`computeSpotlightFactor`, >> `computeSpotlightFactor2` and `computeSpotlightFactor3`) trying out >> different branching and shortcuts. >> >> ### Results >> The CPU "optimizations" made no difference, which is understandable >> considering it will not be the bottleneck. We can remove these if we want to >> simplify, though maybe if we allow a large number of lights it could make a >> difference (I doubt it). I don't have a strong preference either way. >> >> The sphere 1000 tests always gave max fps (120 on Win and 60 on Ubuntu). >> >> **Win 10** >> Compared with the `master` branch, this patch shows 5-10 fps drop in the >> mesh 200 test and ~5 in the mesh 5000 test. I repeated the tests on several >> occasions and got different results in terms of absolute numbers, but the >> relative performance difference remained more or less the same. Out of the 3 >> `computeSpotlightFactor` methods, `computeSpotlightFactor3`, which has no >> "optimizations", gives slightly better performance. >> >> **Ubuntu 18** >> The mesh 200 test always gave 60 fps because it is locked to this fps, so we >> can't measure the real GPU performance change. >> The mesh 5000 test shows 2-6 fps drop from master, with >> `computeSpotlightFactor` > `computeSpotlightFactor2` > >> `computeSpotlightFactor3` at in terms of performance (~2 fps difference >> each). >> >> **Conclusion**: we can expect a 5 fps drop more or less with 3 point lights. >> `computeSpotlightFactor3` on d3d and `computeSpotlightFactor` on opengl gave >> the best performances. > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Update documentation Suggested few doc changes. I have little concern about how the angle of cone/illumination of point is described. Have put a comment for innerAngle description and similar would apply to outer angle. modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGPointLight.java line 48: > 46: /** > 47: * The direction of a {@code SpotLight} that simulates a {@code > PointLight}. > 48: * Since the light radiates equaly in all directions, this value is > meaningless. minor: equaly -> equally modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGShape3D.java line 167: > 165: // float attenuationFactor = 1/(c + cL * d + cQ * d * d); > 166: // float intensity = rL * 0.299f + gL * 0.587f + bL * > 0.114f; > 167: // intensity *= attenuationFactor; minor: align the `//` or remove them. modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 48: > 46: * rotated 90 degrees on the y axis, it will point in the {@code (1, 1, > -1)} direction. > 47: * <p> > 48: * In addition to the attenuation factors defined in {@code PointLight}, > the {@code Spotlight}'s intensity is also {@code Spotlight}'s -> {@code Spot**L**ight}'s modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 49: > 47: * <p> > 48: * In addition to the attenuation factors defined in {@code PointLight}, > the {@code Spotlight}'s intensity is also > 49: * affected by 3 factors: I think instead of phrasing it as "**affected** by 3 factors", it should be phrased as something like, `the SpotLight is defined by 3 more properties` Or `the SpotLight possesses 3 more properties` ? modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 142: > 140: > 141: /** > 142: * The angle of the spotlight's inner cone. Surfaces whose angle to > the light's origin is less than this angle angle of a cone is angle between the two sides of cone or the total angle of cone. Here we are referring to an angle between the Light's direction vector and any side of cone. which would be half of the angle of a cone. So should we mention it here ? 1. Half of the inner cone angle Or 2. Angle between the direction vector of light and edge/side of cone. Am I over thinking here ? Or we should not describe the behavior in terms of angle but mention it in terms of location/distance of a point. If point is inside the cone or outside the cone, distance of the point from the centre of cone. modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 143: > 141: /** > 142: * The angle of the spotlight's inner cone. Surfaces whose angle to > the light's origin is less than this angle > 143: * receive the full light's intensity. At larger angles, the light > intensity starts to drop. The valid range is The light is brightest at the centre of the cone(inner cone), and light intensity won't be same for all points that fall inside the inner cone. Light intensity reduces as the distance of point from centre of cone increases. so the statement "Surfaces whose angle to the light's origin is less than this angle receive the full light's intensity" won't be accurate. Also I would suggest to mention in terms of point instead of surface. modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 171: > 169: * The angle of the spotlight's outer cone. Surfaces whose angle to > the light's origin is greater than this angle > 170: * receive no light. At smaller angles, the light intensity starts > to increase. The valid range is > 171: * {@code innerAngle <= outerAngle <= 180}; values outside of this > range can produce unexpected results. Similar to innerAngle comment. ------------- Changes requested by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/334
