On Sun, 17 Nov 2019 04:15:34 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
> CSR: https://bugs.openjdk.java.net/browse/JDK-8218264 I have added few comments, but have not run tests and sample yet. modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGPointLight.java line 71: > 70: public void setCa(double ca) { > 71: this.ca = ca; > 72: visualsChanged(); `if (this.ca != ca)` check can be added here. Similar for `la`, `qa` and `maxRange` modules/javafx.graphics/src/main/java/com/sun/prism/d3d/D3DContext.java line 556: > 555: void setPointLight(long nativeMeshView, int index, float x, float y, > float z, > 556: float r, float g, float b, float w, float ca, float la, > float qa, float maxRange) { > 557: nSetPointLight(pContext, nativeMeshView, index, x, y, z, r, g, > b, w, ca, la, qa, maxRange); There is an extra space before `float maxRange` modules/javafx.graphics/src/main/java/javafx/scene/PointLight.java line 97: > 96: * The maximum range of this {@code PointLight}. For a pixel to be > affected by this light, its distance to > 97: * the light source must be less than or equal to the light's maximum > range. Any negative value will treated > 98: * as 0. => will `be` treated as 0. modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGPointLight.java line 43: > 42: private static final double DEFAULT_MAX_RANGE = > Double.POSITIVE_INFINITY; > 43: > 44: public NGPointLight() { Will it be a good idea to move these constants to `PointLight` class? However they look good here too. modules/javafx.graphics/src/main/java/javafx/scene/PointLight.java line 41: > 40: * unless it belongs to a {@code Shape3D} outside of its {@code scope}. > 41: * <p> > 42: * The light's intensity can be set to decrease over distance by > attenuating it. The attenuation formula Can the behavior be explained in terms of `Shape `or `Node `instead of `Pixel`. May be something like this, `Any node within the range of the light will be illuminated by this light, except the nodes that are added to the exclusion scope of this light.` modules/javafx.graphics/src/main/java/javafx/scene/PointLight.java line 103: > 102: * {@code maxRange} value by finding the where the attenuation is > close enough to 0. > 103: * <p> > 104: * Nodes that are inside the light's range can still be excluded > from the light's effect {@code maxRange} value by finding the `distance` where the attenuation is close enough to 0. modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGPointLight.java line 64: > 63: > 64: private double ca = DEFAULT_CA; > 65: The coefficients are not directly used in any arithmetic on java side. They are converted to `float` and passed to GL or D3D pipeline. Should these be `float `instead of `double` ? modules/javafx.graphics/src/main/native-prism-d3d/D3DMeshView.cc line 175: > 174: lightsAttenuation[a++] = lights[i].attenuation[2]; > 175: lightsAttenuation[a++] = 0; > 176: In the initial commit this filed was used for `maxRange`. I think that was good idea. Was there any issue with that approach ? ------------- Changes requested by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/43
