Re: RFR: 8217472: Add attenuation for PointLight [v14]
On Fri, 9 Oct 2020 01:39:23 GMT, Nir Lisker wrote:
>> modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.cc line 64:
>>
>>> 62: }
>>> 63:
>>> 64: /*void D3DLight::setRange(float r) {
>>
>> Remove this unused function?
>
> I forgot to mention this point. These setter functions for color and position
> (and range) are never used since whenever
> there is a change in the java side the whole array and lights are recreated
> instead of being adjusted for the change.
> I'm not familiar enough with the memory model of JNI, but it seems expensive
> to re-render everything on every change (I
> think that the mesh is also recreated every time in the native code). Is this
> the way it's supposed to work?
If the lights array is being recreated on a change, there might be a small
savings to be had to reuse the existing
array (it would need to be updated only during the sync while the renderer is
idle).
If the mesh is always being recreated, there may be an opportunity for an even
bigger savings, presuming that only the
mesh data changed (and not the index values in the faces array).
This could be a future optimization, but we would need something to quantify
the possible savings.
-
PR: https://git.openjdk.java.net/jfx/pull/43
Re: RFR: 8217472: Add attenuation for PointLight [v14]
On Thu, 8 Oct 2020 22:21:06 GMT, Kevin Rushforth wrote:
>> Nir Lisker has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Change range after clamping
>
> modules/javafx.graphics/src/main/native-prism-d3d/D3DLight.cc line 64:
>
>> 62: }
>> 63:
>> 64: /*void D3DLight::setRange(float r) {
>
> Remove this unused function?
I forgot to mention this point. These setter functions for color and position
(and range) are never used since whenever
there is a change in the java side the whole array and lights are recreated
instead of being adjusted for the change.
I'm not familiar enough with the memory model of JNI, but it seems expensive to
re-render everything on every change (I
think that the mesh is also recreated every time in the native code). Is this
the way it's supposed to work?
-
PR: https://git.openjdk.java.net/jfx/pull/43
Re: RFR: 8217472: Add attenuation for PointLight [v14]
On Thu, 8 Oct 2020 22:20:17 GMT, Kevin Rushforth wrote: >> Nir Lisker has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Change range after clamping > > 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. Yes, these values are always overridden. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: RFR: 8217472: Add attenuation for PointLight [v14]
On Thu, 24 Sep 2020 02:46:31 GMT, Nir Lisker 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
tests/system/src/test/java/test/javafx/scene/lighting3D/PointLightAttenuationTest.java
line 65:
> 63:
> 64: @BeforeClass
> 65: public static void initFX() throws Exception {
One more thing I forgot to mention. You will need to skip the test if 3D is not
supported using `assumeTrue`. See
[TriangleMeshValidationTest.java#L64](https://github.com/openjdk/jfx/blob/master/tests/system/src/test/java/test/robot/test3d/TriangleMeshValidationTest.java#L64).
-
PR: https://git.openjdk.java.net/jfx/pull/43
Re: RFR: 8217472: Add attenuation for PointLight [v14]
On Thu, 24 Sep 2020 02:46:31 GMT, Nir Lisker 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
Re: RFR: 8217472: Add attenuation for PointLight [v14]
> 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 - Changes: - all: https://git.openjdk.java.net/jfx/pull/43/files - new: https://git.openjdk.java.net/jfx/pull/43/files/af0b1794..21f91dd7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=43=13 - incr: https://webrevs.openjdk.java.net/?repo=jfx=43=12-13 Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/43.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/43/head:pull/43 PR: https://git.openjdk.java.net/jfx/pull/43
