On Fri, 8 May 2020 12:36:41 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8218264 > > Nir Lisker has updated the pull request with a new target base due to a merge > or a rebase. The pull request now > contains 11 commits: > - Merge branch 'master' into 8217472_Add_attenuation_for_PointLight > - Attenuation and range changed internally to floats from doubles > - Fixed shader compilation errors for 2 and 3 lights in es2 > - Addressing review comments > - Fixed whitespaces > - Correction for indexes > - Docs and year update > - Merge remote-tracking branch > 'nlisker/8217472_Add_attenuation_for_PointLight' into > 8217472_Add_attenuation_for_PointLight > - GL pipeline > - Separate range from attenuation > - ... and 1 more: > https://git.openjdk.java.net/jfx/compare/4ec163df...2e1223ed High-level comments: 1. The API docs look good. If you change the public API to `@since 16` then you can also update the CSR and move it to the "Submitted" state. 2. I think it would be good to cleanup the performance test and make it part of this PR, maybe in `tests/performance` (which currently only has the not-very-useful `VMPerformance` subdir) or `tests/manual`. 3. We need some functional tests, ideally automated ones in `tests/system` I also left a few inline comments. I haven't reviewed the shaders yet, so I do that, in addition to further testing, next week. modules/javafx.graphics/src/main/java/com/sun/javafx/scene/PointLightHelper.java line 2: > 1: /* > 2: * Copyright (c) 2016, 2019, Oracle and/or its affiliates. All rights > reserved. > 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. Update the "last modified year" for 2020 or else revert (this applies to all copyright year updates). modules/javafx.graphics/src/main/java/com/sun/javafx/scene/PointLightHelper.java line 77: > 76: } > 77: } Minor: missing newline here modules/javafx.graphics/src/main/java/com/sun/javafx/sg/prism/NGShape3D.java line 187: > 186: 0, 0, 0, 0, // r g b w > 187: 1, 0, 0, 0); // ca la qa maxRange > 188: } Minor: maybe use the getDefaultXxxx methods of NGPointLight? modules/javafx.graphics/src/main/java/javafx/scene/PointLight.java line 51: > 50: * results in the light's color being subtracted from the material > instead of added to it, thus creating a > 51: * "shadow caster". > 52: * <p> Can you think of any problems that might arise by supporting negative coefficients? If not, then this seems fine. modules/javafx.graphics/src/main/java/javafx/scene/PointLight.java line 108: > 107: * @defaultValue {@code Double.POSITIVE_INFINITY} > 108: * @since 14 > 109: */ `@since 16` ------------- PR: https://git.openjdk.java.net/jfx/pull/43
