On Thu, 24 Sep 2020 02:46:31 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8218264 > > Nir Lisker has updated the pull request incrementally with one additional > commit since the last revision: > > Change range after clamping The public API and implementation looks good. I left a few inline comments. I'll review the CSR in parallel with your fixing the few things I noted. modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGPointLight.java line 118: > 116: } > 117: } > 118: } Add newline here. modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.cc line 49: > 47: attenuation[1] = 0; > 48: attenuation[2] = 0; > 49: maxRange = 0; Same comment as in `NGShape3D` about using infinity here, although if this value is always set from the Java side, then this native default might not matter. modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.cc line 64: > 62: } > 63: > 64: /*void D3DLight::setRange(float r) { Remove this unused function? modules/javafx.graphics/src/main/native-prism-es2/GLContext.c line 2128: > 2126: meshViewInfo->pointLightAttenuation[1] = 0; > 2127: meshViewInfo->pointLightAttenuation[2] = 0; > 2128: meshViewInfo->pointLightMaxRange = 0; Same comment as `D3DLight.cc` about the default value. tests/system/src/test/java/test/javafx/scene/lighting3D/PointLightAttenuationTest.java line 51: > 49: public class PointLightAttenuationTest { > 50: > 51: private static final double DELTA = 1d/255; // smallest color > resolution This test fails on my MacBook Pro system. Our other robot-based tests use a higher tolerance because of various problems we run into trying to look for exact values. I recommend a tolerance of `10d/255`, which will still catch whether or not attenuation is used. tests/system/src/test/java/test/javafx/scene/lighting3D/PointLightAttenuationTest.java line 105: > 103: double attenBlueDiag = > snapshot.getPixelReader().getColor(SAMPLE_DIST, 0).getBlue(); > 104: assertEquals("Wrong color value", nonAttenBlueCenter * > attnCenter, attenBlueCenter, DELTA); > 105: assertEquals("Wrong color value", nonAttenBlueDiag * > attnDiag, attenBlueDiag, DELTA); This test looks good. Maybe you could also add a test of quadratic attenuation and max range? ------------- PR: https://git.openjdk.java.net/jfx/pull/43
