RFR: 8217472: Add attenuation for PointLight [v14]
Kevin Rushforth
kcr at openjdk.java.net
Thu Oct 8 23:54:27 UTC 2020
On Thu, 24 Sep 2020 02:46:31 GMT, Nir Lisker <nlisker at 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
More information about the openjfx-dev
mailing list