On Sun, 30 May 2021 05:05:12 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? >> >> - [x] 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: > > Updated image and docs Provided few more suggestions for class doc. Also I noticed that we are not providing a position property for both Point and Spot light. However as of now it seems not required. Should we consider to add it in future ? modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 44: > 42: * A {@code SpotLight} is a {@code PointLight} that radiates light in a > cone in a specific direction. The direction is > 43: * defined by the {@link #directionProperty() direction} vector property > relative to the rotation of the > 44: * {@code SpotLight} that is set with the rotation transforms of the > node. For example, if the direction vector is may be rephrase as -> `The direction is defined by the {@link #directionProperty() direction} vector property of the light. The direction can be rotated by setting a rotation transform on the {@code SpotLight}.` modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 145: > 143: * The angle of the spotlight's inner cone (as shown in the class > doc image). A point whose angle to the light's > 144: * origin is less than this angle is not attenuated by the spotlight > factor ({@code spot = 1}). At larger angles, > 145: * the light intensity starts to drop. The intensity inside inner cone is not attenuated, so the drop in intensity happens only in outer cone. Then the statement `At larger angles, the light intensity starts to drop.` may not be needed here. The drop should be described only with outer angle. modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 173: > 171: > 172: /** > 173: * The angle of the spotlight's outer cone (as shown in the class > doc image). A point whose angle to the light's Let's include the unit **degrees** -> `The angle of the spotlight's outer cone, in degrees (as ...` similar change for innerAngle. modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 175: > 173: * The angle of the spotlight's outer cone (as shown in the class > doc image). A point whose angle to the light's > 174: * origin is greater than this angle receive no light ({@code spot = > 0}). At smaller angles, the light intensity > 175: * starts to increase. 1. minor typo: receive -> receives. 2. At smaller angles, the light intensity starts to increase -> I think the statement should be rephrased similar to falloff statement. May be re-use a statement from falloff doc, with minor changes, `A point whose angle to the light's position is greater than the inner angle but less than the outer angle receives partial intensity governed by the falloff factor` modules/javafx.graphics/src/main/java/javafx/scene/SpotLight.java line 203: > 201: > 202: /** > 203: * The intensity falloff factor of the spotlight's outer cone. A > point whose angle to the light's origin is greater During this doc the position of light is referred using two different words `origin` and `source` ( Line 58, in class doc ). I would recommend to use word `position` uniformly. modules/javafx.graphics/src/main/resources/com/sun/prism/es2/glsl/main2Lights.frag line 76: > 74: varying vec4 lightTangentSpaceDirections[3]; > 75: > 76: // Because pow(0, 0) is undefined > (https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-pow), This is hlsl reference link, may be we should find a similar link for glsl or remove this reference. ------------- Changes requested by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/334
