Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v13]
On Thu, 28 Oct 2021 13:50:40 GMT, Andreas Heger wrote: >> The inconsistent illumination happens on Macs with retina displays only if >> the 3D shape is placed in a SubScene. The light sources are located with >> wrong coordinates in sub scenes and this causes a different illumination. >> The wrong coordinates for the light sources come from the fact that the >> retina pixel scale factors are not used in a SubScene. >> >> With this pull request, the retina pixel scale factors will be also used in >> SubScenes and this should resolve the bug >> [https://bugs.openjdk.java.net/browse/JDK-8255015](url) > > Andreas Heger has updated the pull request incrementally with one additional > commit since the last revision: > > 8255015: Copyright year and wrong spelling in comment corrected Marked as reviewed by arapte (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v13]
On Thu, 28 Oct 2021 13:50:40 GMT, Andreas Heger wrote: >> The inconsistent illumination happens on Macs with retina displays only if >> the 3D shape is placed in a SubScene. The light sources are located with >> wrong coordinates in sub scenes and this causes a different illumination. >> The wrong coordinates for the light sources come from the fact that the >> retina pixel scale factors are not used in a SubScene. >> >> With this pull request, the retina pixel scale factors will be also used in >> SubScenes and this should resolve the bug >> [https://bugs.openjdk.java.net/browse/JDK-8255015](url) > > Andreas Heger has updated the pull request incrementally with one additional > commit since the last revision: > > 8255015: Copyright year and wrong spelling in comment corrected Marked as reviewed by kcr (Lead). - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v13]
> The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) Andreas Heger has updated the pull request incrementally with one additional commit since the last revision: 8255015: Copyright year and wrong spelling in comment corrected - Changes: - all: https://git.openjdk.java.net/jfx/pull/531/files - new: https://git.openjdk.java.net/jfx/pull/531/files/1598c604..9c10d967 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=531=12 - incr: https://webrevs.openjdk.java.net/?repo=jfx=531=11-12 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/531.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/531/head:pull/531 PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v12]
On Wed, 27 Oct 2021 09:36:20 GMT, Ambarish Rapte wrote: >> Andreas Heger has updated the pull request incrementally with one additional >> commit since the last revision: >> >> 8255015: testScene variable must be volatile and new line at the end of >> the file added > > tests/system/src/test/java/test/robot/test3d/PointLightIlluminationTest.java > line 148: > >> 146: * Creates a new scene with a subscene which contains a perspective >> camera and a sphere >> 147: * Although this test class checks the pointlight illumination, >> there is no explicit pointlight >> 148: * in the scene. For the test, it is sufficient to use the default >> pointlight which is created > > minor: Please include this small correction along with the copyright year > change. > pointlight -> point light Thanks for the review! I corrected the copyright year and the point light spelling. - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v12]
On Tue, 26 Oct 2021 19:48:41 GMT, Andreas Heger wrote: >> The inconsistent illumination happens on Macs with retina displays only if >> the 3D shape is placed in a SubScene. The light sources are located with >> wrong coordinates in sub scenes and this causes a different illumination. >> The wrong coordinates for the light sources come from the fact that the >> retina pixel scale factors are not used in a SubScene. >> >> With this pull request, the retina pixel scale factors will be also used in >> SubScenes and this should resolve the bug >> [https://bugs.openjdk.java.net/browse/JDK-8255015](url) > > Andreas Heger has updated the pull request incrementally with one additional > commit since the last revision: > > 8255015: testScene variable must be volatile and new line at the end of the > file added Looks good to me. The copyright year in test file needs to be changed. Along with it please fix the suggested minor typo. tests/system/src/test/java/test/robot/test3d/PointLightIlluminationTest.java line 2: > 1: /* > 2: * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights > reserved. It should have only one copyright year: 2021. -> `* Copyright (c) 2021, Oracle and/o` tests/system/src/test/java/test/robot/test3d/PointLightIlluminationTest.java line 148: > 146: * Creates a new scene with a subscene which contains a perspective > camera and a sphere > 147: * Although this test class checks the pointlight illumination, > there is no explicit pointlight > 148: * in the scene. For the test, it is sufficient to use the default > pointlight which is created minor: Please include this small correction along with the copyright year change. pointlight -> point light - Changes requested by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v12]
On Tue, 26 Oct 2021 19:48:41 GMT, Andreas Heger wrote: >> The inconsistent illumination happens on Macs with retina displays only if >> the 3D shape is placed in a SubScene. The light sources are located with >> wrong coordinates in sub scenes and this causes a different illumination. >> The wrong coordinates for the light sources come from the fact that the >> retina pixel scale factors are not used in a SubScene. >> >> With this pull request, the retina pixel scale factors will be also used in >> SubScenes and this should resolve the bug >> [https://bugs.openjdk.java.net/browse/JDK-8255015](url) > > Andreas Heger has updated the pull request incrementally with one additional > commit since the last revision: > > 8255015: testScene variable must be volatile and new line at the end of the > file added Looks good. I ran the test on all three platforms. On macOS I ran the test on both a retina and non-retina screen. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v11]
On Mon, 25 Oct 2021 23:50:09 GMT, Kevin Rushforth wrote: >> Andreas Heger has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 10 additional >> commits since the last revision: >> >> - Merge branch 'openjdk:master' into fix-8255015 >> - 8255015: Comments corrected >> - 8255015: Comment about copying pixel scale factors corrected >> - 8255015: Tabs removed from PointLightIllumination.java >> - Merge branch 'openjdk:master' into fix-8255015 >> - 8255015: JUnit Test class added. >> - Merge branch 'openjdk:master' into fix-8255015 >> - Merge branch 'openjdk:master' into fix-8255015 >> - Merge branch 'openjdk:master' into fix-8255015 >> - 8255015: Copy pixel scale factors from graphics object to subscene >> graphics so that the position of lights will be scaled correctly on retina >> displays > > tests/system/src/test/java/test/robot/test3d/PointLightIlluminationTest.java > line 67: > >> 65: private static final intLOWER_CORNER_Y = (int) >> (SCENE_WIDTH_HEIGHT * (1 - CORNER_FACTOR)); >> 66: private static final double COLOR_TOLERANCE= 0.07; >> 67: private static Scene testScene; > > This is created on one thread and tested on another (to see whether it's > already been created), so I recommend making it `volatile` (i.e., `private > static volatile ...`). Also, you might want to explicitly set it to `null` > since you rely on it (yes, I know `null` is the default). @kevinrushforth Thanks for the hint about about making the variable volatile! I've just updated the class accordingly. - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v12]
> The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) Andreas Heger has updated the pull request incrementally with one additional commit since the last revision: 8255015: testScene variable must be volatile and new line at the end of the file added - Changes: - all: https://git.openjdk.java.net/jfx/pull/531/files - new: https://git.openjdk.java.net/jfx/pull/531/files/656cb7d8..1598c604 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=531=11 - incr: https://webrevs.openjdk.java.net/?repo=jfx=531=10-11 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/531.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/531/head:pull/531 PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v11]
On Sun, 24 Oct 2021 17:23:40 GMT, Andreas Heger wrote: >> The inconsistent illumination happens on Macs with retina displays only if >> the 3D shape is placed in a SubScene. The light sources are located with >> wrong coordinates in sub scenes and this causes a different illumination. >> The wrong coordinates for the light sources come from the fact that the >> retina pixel scale factors are not used in a SubScene. >> >> With this pull request, the retina pixel scale factors will be also used in >> SubScenes and this should resolve the bug >> [https://bugs.openjdk.java.net/browse/JDK-8255015](url) > > Andreas Heger has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 10 additional > commits since the last revision: > > - Merge branch 'openjdk:master' into fix-8255015 > - 8255015: Comments corrected > - 8255015: Comment about copying pixel scale factors corrected > - 8255015: Tabs removed from PointLightIllumination.java > - Merge branch 'openjdk:master' into fix-8255015 > - 8255015: JUnit Test class added. > - Merge branch 'openjdk:master' into fix-8255015 > - Merge branch 'openjdk:master' into fix-8255015 > - Merge branch 'openjdk:master' into fix-8255015 > - 8255015: Copy pixel scale factors from graphics object to subscene > graphics so that the position of lights will be scaled correctly on retina > displays The fix and the test looks good. I left a couple minor comments on the test. I ran the test on macOS with retina. I'll double-check on the other platforms, but I think this is about ready to go. tests/system/src/test/java/test/robot/test3d/PointLightIlluminationTest.java line 67: > 65: private static final intLOWER_CORNER_Y = (int) > (SCENE_WIDTH_HEIGHT * (1 - CORNER_FACTOR)); > 66: private static final double COLOR_TOLERANCE= 0.07; > 67: private static Scene testScene; This is created on one thread and tested on another (to see whether it's already been created), so I recommend making it `volatile` (i.e., `private static volatile ...`). Also, you might want to explicitly set it to `null` since you rely on it (yes, I know `null` is the default). tests/system/src/test/java/test/robot/test3d/PointLightIlluminationTest.java line 180: > 178: return new Scene(new Group(subScene), SCENE_WIDTH_HEIGHT, > SCENE_WIDTH_HEIGHT); > 179: } > 180: } Minor: there should be a new line at the end of the file. - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight
On Sun, 24 Oct 2021 16:04:40 GMT, Andreas Heger wrote: > I tried to use the Node.snapshot method, but in this case the taken snapshot > always had the correct illumination, no matter what display was used and even > though the scene on the display showed the wrong illumination. I guess the > snapshot method renders the node directly into an image and does not use the > physical screen content and so the pixel scale factor does not play any role > in this case. Maybe this is why the [lighting system tests](https://github.com/openjdk/jfx/tree/master/tests/system/src/test/java/test/javafx/scene/lighting3D) pass on Retina screens. - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v11]
> The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) Andreas Heger has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 10 additional commits since the last revision: - Merge branch 'openjdk:master' into fix-8255015 - 8255015: Comments corrected - 8255015: Comment about copying pixel scale factors corrected - 8255015: Tabs removed from PointLightIllumination.java - Merge branch 'openjdk:master' into fix-8255015 - 8255015: JUnit Test class added. - Merge branch 'openjdk:master' into fix-8255015 - Merge branch 'openjdk:master' into fix-8255015 - Merge branch 'openjdk:master' into fix-8255015 - 8255015: Copy pixel scale factors from graphics object to subscene graphics so that the position of lights will be scaled correctly on retina displays - Changes: - all: https://git.openjdk.java.net/jfx/pull/531/files - new: https://git.openjdk.java.net/jfx/pull/531/files/cc995300..656cb7d8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=531=10 - incr: https://webrevs.openjdk.java.net/?repo=jfx=531=09-10 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/531.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/531/head:pull/531 PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight
On Mon, 20 Sep 2021 14:11:50 GMT, Kevin Rushforth wrote: >> @kevinrushforth >>> The fix looks good. I tested it both in isolation and with PR #334 and it >>> works on both a retina and non-retina display. >>> >>> If you have time to write an automated test, that would be useful, but if >>> not then a manual test would be OK. >> >> Ok, I will try to write an automated test case which draws a sphere in a >> SubScene and then calculates the average color of the generated image. The >> test will be passed if the calculated average does not differ from the >> excepted average color by a certain tolerance value. I'm not sure if I will >> manage to do this... but I will give it a try. > > @andreas-heger are you able to come up with an automated test for this bug? @kevinrushforth I've finally added a JUnit test to verify the illumination. I tested it on my MacBook with and without Retina display and also on my linux system with non HiDPI display. It only fails on the MacBook with retina display, if my fix isn't used. In all other situations (non retina display and no matter if the fix is used or not) it succeeds. In my test class PointLightIlluminationTest, I had to use the Robot API for getting the pixel colors from the screen. In a first attempt, I tried to use the Node.snapshot method, but in this case the taken snapshot always had the correct illumination, no matter what display was used and even though the scene on the display showed the wrong illumination. I guess the snapshot method renders the node directly into an image and does not use the physical screen content and so the pixel scale factor does not play any role in this case. So, I placed the test class into the package test.robot.test3d and derived it from the existing class VisualTestBase. There are 6 tests in the class. The first checks the background color of the subscene (this test should be always succeed, no matter if the fix is applied or not... I added is just to have more security that the whole scene is really setup as desired). The other tests check the color of the displayed sphere at 4 corner pixel and one in the ce nter. I hope that this test can be useful for this project. There is one more point, not related to this pull request. I found out that the JFX project does not compile on an apple silicon mac. The -arch option of the clang compiler does not accept the value "aarch64". It must be "arm64" so that the JFX project compiles on my M1 MacBook Air. I will search if there is already a bug or create a new one in the next days. - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v10]
> The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) Andreas Heger has updated the pull request incrementally with one additional commit since the last revision: 8255015: Comments corrected - Changes: - all: https://git.openjdk.java.net/jfx/pull/531/files - new: https://git.openjdk.java.net/jfx/pull/531/files/280ae78c..cc995300 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=531=09 - incr: https://webrevs.openjdk.java.net/?repo=jfx=531=08-09 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jfx/pull/531.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/531/head:pull/531 PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v9]
> The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) Andreas Heger has updated the pull request incrementally with one additional commit since the last revision: 8255015: Comment about copying pixel scale factors corrected - Changes: - all: https://git.openjdk.java.net/jfx/pull/531/files - new: https://git.openjdk.java.net/jfx/pull/531/files/b0b6c13b..280ae78c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=531=08 - incr: https://webrevs.openjdk.java.net/?repo=jfx=531=07-08 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jfx/pull/531.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/531/head:pull/531 PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v8]
> The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) Andreas Heger has updated the pull request incrementally with one additional commit since the last revision: 8255015: Tabs removed from PointLightIllumination.java - Changes: - all: https://git.openjdk.java.net/jfx/pull/531/files - new: https://git.openjdk.java.net/jfx/pull/531/files/4168f5f3..b0b6c13b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=531=07 - incr: https://webrevs.openjdk.java.net/?repo=jfx=531=06-07 Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jfx/pull/531.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/531/head:pull/531 PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v7]
> The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) Andreas Heger has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Merge branch 'openjdk:master' into fix-8255015 - 8255015: JUnit Test class added. - Merge branch 'openjdk:master' into fix-8255015 - Merge branch 'openjdk:master' into fix-8255015 - Merge branch 'openjdk:master' into fix-8255015 - 8255015: Copy pixel scale factors from graphics object to subscene graphics so that the position of lights will be scaled correctly on retina displays - Changes: - all: https://git.openjdk.java.net/jfx/pull/531/files - new: https://git.openjdk.java.net/jfx/pull/531/files/3e1af1c3..4168f5f3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=531=06 - incr: https://webrevs.openjdk.java.net/?repo=jfx=531=05-06 Stats: 4396 lines in 237 files changed: 3132 ins; 530 del; 734 mod Patch: https://git.openjdk.java.net/jfx/pull/531.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/531/head:pull/531 PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v6]
> The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) Andreas Heger has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: 8255015: JUnit Test class added. - Changes: - all: https://git.openjdk.java.net/jfx/pull/531/files - new: https://git.openjdk.java.net/jfx/pull/531/files/a3677124..3e1af1c3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=531=05 - incr: https://webrevs.openjdk.java.net/?repo=jfx=531=04-05 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/531.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/531/head:pull/531 PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v5]
> The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) Andreas Heger has updated the pull request incrementally with one additional commit since the last revision: JUnit Test class for verifying JDK-8255015 added. - Changes: - all: https://git.openjdk.java.net/jfx/pull/531/files - new: https://git.openjdk.java.net/jfx/pull/531/files/c7e58901..a3677124 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=531=04 - incr: https://webrevs.openjdk.java.net/?repo=jfx=531=03-04 Stats: 180 lines in 1 file changed: 180 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/531.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/531/head:pull/531 PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight
On Mon, 20 Sep 2021 14:11:50 GMT, Kevin Rushforth wrote: >> @kevinrushforth >>> The fix looks good. I tested it both in isolation and with PR #334 and it >>> works on both a retina and non-retina display. >>> >>> If you have time to write an automated test, that would be useful, but if >>> not then a manual test would be OK. >> >> Ok, I will try to write an automated test case which draws a sphere in a >> SubScene and then calculates the average color of the generated image. The >> test will be passed if the calculated average does not differ from the >> excepted average color by a certain tolerance value. I'm not sure if I will >> manage to do this... but I will give it a try. > > @andreas-heger are you able to come up with an automated test for this bug? @kevinrushforth yes, I still want to write an automated test for this bug. Sorry for the delay... I plan to do it within the next two weeks. - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight
On Mon, 14 Jun 2021 11:05:25 GMT, Andreas Heger wrote: >> @andreas-heger Welcome to the `jfx` project. At a quick glance, the fix >> looks promising. Have you tested this on Windows with Hi-DPI to make sure >> there is no impact? Would you be able add an automated test case that fails >> (only on Mac retina) without the fix and passes (on all platforms) with your >> fix? Hi-DPI fixes are often tricky to test in an automated test, so if not, >> we can use the existing manual test. >> >> @nlisker this is the same problem I noted while testing PR #334. Clearly I >> had forgotten that it was not only a preexisting bug, but a known bug that >> was already filed. I intend to test this alone and in connection with your >> PR. > > @kevinrushforth >> The fix looks good. I tested it both in isolation and with PR #334 and it >> works on both a retina and non-retina display. >> >> If you have time to write an automated test, that would be useful, but if >> not then a manual test would be OK. > > Ok, I will try to write an automated test case which draws a sphere in a > SubScene and then calculates the average color of the generated image. The > test will be passed if the calculated average does not differ from the > excepted average color by a certain tolerance value. I'm not sure if I will > manage to do this... but I will give it a try. @andreas-heger are you able to come up with an automated test for this bug? - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v4]
> The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) Andreas Heger has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge branch 'openjdk:master' into fix-8255015 - Merge branch 'openjdk:master' into fix-8255015 - Merge branch 'openjdk:master' into fix-8255015 - 8255015: Copy pixel scale factors from graphics object to subscene graphics so that the position of lights will be scaled correctly on retina displays - Changes: - all: https://git.openjdk.java.net/jfx/pull/531/files - new: https://git.openjdk.java.net/jfx/pull/531/files/fdb1ddbe..c7e58901 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=531=03 - incr: https://webrevs.openjdk.java.net/?repo=jfx=531=02-03 Stats: 179 lines in 6 files changed: 156 ins; 18 del; 5 mod Patch: https://git.openjdk.java.net/jfx/pull/531.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/531/head:pull/531 PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v3]
> The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) Andreas Heger has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge branch 'openjdk:master' into fix-8255015 - Merge branch 'openjdk:master' into fix-8255015 - 8255015: Copy pixel scale factors from graphics object to subscene graphics so that the position of lights will be scaled correctly on retina displays - Changes: - all: https://git.openjdk.java.net/jfx/pull/531/files - new: https://git.openjdk.java.net/jfx/pull/531/files/3973a2e2..fdb1ddbe Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=531=02 - incr: https://webrevs.openjdk.java.net/?repo=jfx=531=01-02 Stats: 5975 lines in 27 files changed: 5975 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/531.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/531/head:pull/531 PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v2]
> The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) Andreas Heger has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision: - Merge branch 'openjdk:master' into fix-8255015 - 8255015: Copy pixel scale factors from graphics object to subscene graphics so that the position of lights will be scaled correctly on retina displays - Changes: - all: https://git.openjdk.java.net/jfx/pull/531/files - new: https://git.openjdk.java.net/jfx/pull/531/files/66b361a0..3973a2e2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=531=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=531=00-01 Stats: 344178 lines in 6843 files changed: 186475 ins; 110834 del; 46869 mod Patch: https://git.openjdk.java.net/jfx/pull/531.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/531/head:pull/531 PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight
On Mon, 14 Jun 2021 11:05:25 GMT, Andreas Heger wrote: > Ok, I will try to write an automated test case which draws a sphere in a > SubScene and then calculates the average color of the generated image. The > test will be passed if the calculated average does not differ from the > excepted average color by a certain tolerance value. I'm not sure if I will > manage to do this... but I will give it a try. You can look at such lighting tests in [this PR](https://github.com/nlisker/jfx/tree/8234920_Add_SpotLight_to_the_selection_of_3D_light_types/tests/system/src/test/java/test/javafx/scene/lighting3D). I suspect you will have to do something very similar. Since the scaling is off, I think that you can just sample a few points that have different colors with and without your patch. As noted in the files, you will have to be careful with taking a snapshot in a subscene because of [JDK-8260013](https://bugs.openjdk.java.net/browse/JDK-8260013) (which I have just unassigned from myself because it doesn't seem like I will have time for it). - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight
On Wed, 9 Jun 2021 18:46:16 GMT, Kevin Rushforth wrote: >> The inconsistent illumination happens on Macs with retina displays only if >> the 3D shape is placed in a SubScene. The light sources are located with >> wrong coordinates in sub scenes and this causes a different illumination. >> The wrong coordinates for the light sources come from the fact that the >> retina pixel scale factors are not used in a SubScene. >> >> With this pull request, the retina pixel scale factors will be also used in >> SubScenes and this should resolve the bug >> [https://bugs.openjdk.java.net/browse/JDK-8255015](url) > > @andreas-heger Welcome to the `jfx` project. At a quick glance, the fix looks > promising. Have you tested this on Windows with Hi-DPI to make sure there is > no impact? Would you be able add an automated test case that fails (only on > Mac retina) without the fix and passes (on all platforms) with your fix? > Hi-DPI fixes are often tricky to test in an automated test, so if not, we can > use the existing manual test. > > @nlisker this is the same problem I noted while testing PR #334. Clearly I > had forgotten that it was not only a preexisting bug, but a known bug that > was already filed. I intend to test this alone and in connection with your PR. @kevinrushforth > The fix looks good. I tested it both in isolation and with PR #334 and it > works on both a retina and non-retina display. > > If you have time to write an automated test, that would be useful, but if not > then a manual test would be OK. Ok, I will try to write an automated test case which draws a sphere in a SubScene and then calculates the average color of the generated image. The test will be passed if the calculated average does not differ from the excepted average color by a certain tolerance value. I'm not sure if I will manage to do this... but I will give it a try. - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight
On Wed, 9 Jun 2021 18:22:31 GMT, Andreas Heger wrote: > The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) The fix looks good. I tested it both in isolation and with PR #334 and it works on both a retina and non-retina display. If you have time to write an automated test, that would be useful, but if not then a manual test would be OK. modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGSubScene.java line 211: > 209: // The pixel scale factors must be set to the rttGraphics, > otherwise the position > 210: // of the lights will not be scaled correctly on retina > displays > 211: // See https://bugs.openjdk.java.net/browse/JDK-8255015 We typically don't put pointers to bug IDs in the code (we used to, and still sometimes do in some special cases, but here it isn't needed). - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight
On Wed, 9 Jun 2021 18:46:16 GMT, Kevin Rushforth wrote: > I intend to test this alone and in connection with your PR. Alright. I'll keep an eye on this PR. - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight
On Wed, 9 Jun 2021 18:46:16 GMT, Kevin Rushforth wrote: > @andreas-heger Welcome to the `jfx` project. At a quick glance, the fix looks > promising. Have you tested this on Windows with Hi-DPI to make sure there is > no impact? No, I only have got MacOS on retina / non retina and a non Hi-DPI Ubuntu system available for tests, but no Windows. > Would you be able add an automated test case that fails (only on Mac retina) > without the fix and passes (on all platforms) with your fix? Hi-DPI fixes are > often tricky to test in an automated test, so if not, we can use the existing > manual test. I'm not very experienced in automated tests in general and a test which verifies graphic output might be really challenging. I can take a look at it, but please don't expect any results for the next couple of weeks, or any at all. - PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight
On Wed, 9 Jun 2021 18:22:31 GMT, Andreas Heger wrote: > The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) @andreas-heger Welcome to the `jfx` project. At a quick glance, the fix looks promising. Have you tested this on Windows with Hi-DPI to make sure there is no impact? Would you be able add an automated test case that fails (only on Mac retina) without the fix and passes (on all platforms) with your fix? Hi-DPI fixes are often tricky to test in an automated test, so if not, we can use the existing manual test. @nlisker this is the same problem I noted while testing PR #334. Clearly I had forgotten that it was not only a preexisting bug, but a known bug that was already filed. I intend to test this alone and in connection with your PR. - PR: https://git.openjdk.java.net/jfx/pull/531
RFR: 8255015: Inconsistent illumination of 3D shape by PointLight
The inconsistent illumination happens on Macs with retina displays only if the 3D shape is placed in a SubScene. The light sources are located with wrong coordinates in sub scenes and this causes a different illumination. The wrong coordinates for the light sources come from the fact that the retina pixel scale factors are not used in a SubScene. With this pull request, the retina pixel scale factors will be also used in SubScenes and this should resolve the bug [https://bugs.openjdk.java.net/browse/JDK-8255015](url) - Commit messages: - 8255015: Copy pixel scale factors from graphics object to subscene graphics so that the position of lights will be scaled correctly on retina displays Changes: https://git.openjdk.java.net/jfx/pull/531/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=531=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255015 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/531.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/531/head:pull/531 PR: https://git.openjdk.java.net/jfx/pull/531
Re: RFR: 8255015: Inconsistent illumination of 3D shape by PointLight [v2]
> The inconsistent illumination happens on Macs with retina displays only if > the 3D shape is placed in a SubScene. The light sources are located with > wrong coordinates in sub scenes and this causes a different illumination. The > wrong coordinates for the light sources come from the fact that the retina > pixel scale factors are not used in a SubScene. > > With this pull request, the retina pixel scale factors will be also used in > SubScenes and this should resolve the bug > [https://bugs.openjdk.java.net/browse/JDK-8255015](url) Andreas Heger has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. - Changes: - all: https://git.openjdk.java.net/jfx/pull/530/files - new: https://git.openjdk.java.net/jfx/pull/530/files/66b361a0..ca250364 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=530=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=530=00-01 Stats: 4 lines in 1 file changed: 0 ins; 4 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/530.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/530/head:pull/530 PR: https://git.openjdk.java.net/jfx/pull/530
RFR: 8255015: Inconsistent illumination of 3D shape by PointLight
The inconsistent illumination happens on Macs with retina displays only if the 3D shape is placed in a SubScene. The light sources are located with wrong coordinates in sub scenes and this causes a different illumination. The wrong coordinates for the light sources come from the fact that the retina pixel scale factors are not used in a SubScene. With this pull request, the retina pixel scale factors will be also used in SubScenes and this should resolve the bug [https://bugs.openjdk.java.net/browse/JDK-8255015](url) - Commit messages: - 8255015: Copy pixel scale factors from graphics object to subscene graphics so that the position of lights will be scaled correctly on retina displays Changes: https://git.openjdk.java.net/jfx/pull/530/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=530=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255015 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/530.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/530/head:pull/530 PR: https://git.openjdk.java.net/jfx/pull/530
