On Sun, 3 Mar 2024 22:29:02 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
> Update for the 3D lighting test tool as described in the JBS issue.
providing few more comments
tests/performance/3DLighting/src/main/java/lighting3D/Benchmark.java line 49:
> 47: stopGraphic.setBoundsType(TextBoundsType.VISUAL);
> 48: stopGraphic.setFill(Color.RED);
> 49: stopGraphic.setFont(Font.font(20));
Play and Stop button could be same size. It would be good to look at. Currently
the Stop button seems lesser than half size of play button.
tests/performance/3DLighting/src/main/java/lighting3D/Benchmark.java line 81:
> 79:
> 80: var sphere = new Button("Sphere");
> 81: sphere.setOnAction(e ->
> switchTo(Models.createSphere(SPHERE_RADIUS, (int)
> subdivisionSlider.getValue())));
In `Models.createSphere()` method second argument is used for z distance, and
not for subdivisions.. Increasing the slider value reduces the size of Sphere
and vanishes at 510.
tests/performance/3DLighting/src/main/java/lighting3D/Benchmark.java line 179:
> 177: totalElapsedFrames = 0;
> 178: System.out.println();
> 179: System.out.println(" --------------------- ");
May be print a message in addition to this.
tests/performance/3DLighting/src/main/java/lighting3D/CaptureUtils.java line 29:
> 27: }
> 28:
> 29: private static final Path FOLDER = Path.of("screenshots");
suggestion: FOLDER -> SCREENSHOT_DIR or DIRECTORY
Using word directory would align with Java io/nio naming.
tests/performance/3DLighting/src/main/java/lighting3D/LightingApplication.java
line 62:
> 60: Node lightsControls = environment.createLightsControls();
> 61:
> 62: var controls = new VBox(perfControls, modelsControls,
> backgroundControls, screenshotControls,
Recommend to provide some spacing. 5 looked good.
-> var controls = new VBox(**5**, perfControls, modelsControls,
backgroundControls, screenshotControls,
-------------
Changes requested by arapte (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1387#pullrequestreview-1917586203
PR Review Comment: https://git.openjdk.org/jfx/pull/1387#discussion_r1513164981
PR Review Comment: https://git.openjdk.org/jfx/pull/1387#discussion_r1513182318
PR Review Comment: https://git.openjdk.org/jfx/pull/1387#discussion_r1513870834
PR Review Comment: https://git.openjdk.org/jfx/pull/1387#discussion_r1513876778
PR Review Comment: https://git.openjdk.org/jfx/pull/1387#discussion_r1514693071
