Re: RFR: 8204568: Relative CSS-Attributes don't work all time
On Mon, 8 Feb 2021 11:37:35 GMT, Ambarish Rapte wrote: > Issue is that the size of properties that are relatively(`em`) sized is not > computed correctly when the reference `-fx-font-size` is also specified > relatively and is nested. > > Fix is a slight variation of an earlier suggestion in [the > PR](https://github.com/javafxports/openjdk-jfx/pull/94). > > Fix is very specific to this scenario and did not show any side effect nor > any test failure. > > There are 12 new unit tests added along with fix: > - Two tests fail before and pass after this fix. These test verify the > reported failing scenario. > sameRelativeFontSizeNestedParentTest > relativeFontSizeDeepNestedParentControlTest > - Two other tests fail both before and after this fix. They are not related > to the fix. These two tests are ignored. I shall file new JBS issues to track > these cases and update PR with the IDs added to @Ignore. > propertySizesRelativeToFontSizeOfControlTest > propertySizesRelativeToFontSizeOfParentTest > - Other 8 tests are sanity tests which pass both before and after this fix. This is taking me longer to review than I expected, because I ran into some anomalies while doing some additional testing. There is an unexpected change in behavior for nested panes with relative sizes. We need to understand this change before this is integrated. I added a modified version of the original test program to the JBS bug report that creates a scene graph like this, where the root and both parent nodes specify the font size in absolute pixels: Root // -fx-font-size: 96px P1 // -fx-font-size: 48px P2 // -fx-font-size: 36px L1 (unset), L2 (18px), L3 (0.5em) // All three had padding and bg radius at 0.5em The above scene graph works as expected with the fix, whereas before the fix label L3 had incorrect padding. I then added a button to cycle through 4 modes replacing first P2, then P1, then the Root with what would be "equivalent" relative font sizes if the definition of an "em" is its parent's font size. Root // -fx-font-size: 96px P1 // -fx-font-size: 48px P2 // -fx-font-size: 0.75em L1 (unset), L2 (18px), L3 (0.5em) // All three had padding and bg radius at 0.5em Root // -fx-font-size: 96px P1 // -fx-font-size: 0.5em P2 // -fx-font-size: 0.75em L1 (unset), L2 (18px), L3 (0.5em) // All three had padding and bg radius at 0.5em Root // -fx-font-size: 8.0em P1 // -fx-font-size: 0.5em P2 // -fx-font-size: 0.75em L1 (unset), L2 (18px), L3 (0.5em) // All three had padding and bg radius at 0.5em Things start getting unexpected when there is a parent with a relative font size, and the label had a relative font size (e.g., L3 when P2 is relative). When all nodes are relative (the last case), the padding size is completely wrong. Btw, I'm not currently worried about the calculation of the font size itself; this fix is intended to be a targeted fix that doesn't change the calculated font size (separately, we could look at that, but it would be much riskier and is out of scope for this bug fix). What I want to make sure is that in all cases, specifying the padding or other sizes in a node in ems will be relative to whatever the calculated font size is. - Changes requested by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/397
Re: RFR: 8234920: Add SpotLight to the selection of 3D light types [v7]
On Wed, 3 Feb 2021 21:56:08 GMT, Kevin Rushforth wrote: > I still get a runtime shader compilation error on Mac (see above) I pushed a fix now. Since it doesn't happen on Win or Ubuntu I can't verify it, but I changed `0` to `0.0` etc. in the shader code. > Unless the transform itself is non-invertible, I don't think this can happen. > What I would expect is that if we go this route -- which seems like a fine > API choice to me -- the direction vector would be `(0, 0, 1)` in the local > coordinate system of the light, which would then be suitably rotated based on > the transform of the light (and any transforms above it). I think that this is also what I did when I tested this approach. When computing the direction in `NGSpotLight`, I multiplied the `Affine3D worldTransform` by a `Vec3D` of (0, 0, 1). Then, using rotations on the main axes, I was able to rotate the light in any direction I want. The problem here is ease of use. Because the order in which you add the rotation transforms matter, it becomes unintuitive for the user compared to using a direction. If I rotate 90 degrees in the X direction, the Y and Z axes are "flipped". If we can make it intuitive, then I think this approach is better. > Hmm. I wonder if the ideal choice? One of the reasons for wanting a > DirectionalLight, as distinct from a very far away PointLight with no > attenuation, is performance. On the other hand, since it will complicates the > design of the shaders to have a special case for directional lights, it might > not be worth implementing. I don't think it matters all that much, so if you > want to rename it to setLight that seems OK. It's an implementation detail > and could be revisited in the future. The branching vs unneeded computation costs is a main testing point in this patch. We will do the same tests for DirectionalLight in its own patch. A DirectionalLight can be implemented in the vertex shader, but it gives less accurate results I believe. I updated the Ubuntu tests results. - PR: https://git.openjdk.java.net/jfx/pull/334
Re: RFR: 8259046: ViewPainter.ROOT_PATHS holds reference to Scene causing memory leak
On Wed, 27 Jan 2021 13:31:45 GMT, Kevin Rushforth wrote: > Prism implements a dirty region optimization, where in many cases, only part > of the scene graph is re-rendered when something changes. In support of this, > the `ViewPainter` class in the Quantum Toolkit keeps an array of node paths, > `ROOT_PATHS`, which is a list of sub-trees in the scene graph that need to be > rendered based on a change to that node. The entries in the `ROOT_PATHS` > array are intended to be transient during the rendering of a single pass of a > single scene. They are recreated every time a scene is rendered. The leak > occurs because the entries are not cleared after being used. The fix is to > clear each entry after it is rendered. In addition to static analysis, which > shows that the entries are never used again after a frame is rendered, I have > done a full build and test, including manual tests, to be sure that there is > no regression. > > I have added a test that will fail consistently on Mac and Windows (and > intermittently on Linux) without the fix. It passes consistently on all > platforms with the fix. > > Even though this is a simple change, it is in an area that has historically > been fragile, so I would like two reviewers. Looks good to me. - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/388
Re: RFR: 8204568: Relative CSS-Attributes don't work all time
On Mon, 8 Feb 2021 11:37:35 GMT, Ambarish Rapte wrote: > Issue is that the size of properties that are relatively(`em`) sized is not > computed correctly when the reference `-fx-font-size` is also specified > relatively and is nested. > > Fix is a slight variation of an earlier suggestion in [the > PR](https://github.com/javafxports/openjdk-jfx/pull/94). > > Fix is very specific to this scenario and did not show any side effect nor > any test failure. > > There are 12 new unit tests added along with fix: > - Two tests fail before and pass after this fix. These test verify the > reported failing scenario. > sameRelativeFontSizeNestedParentTest > relativeFontSizeDeepNestedParentControlTest > - Two other tests fail both before and after this fix. They are not related > to the fix. These two tests are ignored. I shall file new JBS issues to track > these cases and update PR with the IDs added to @Ignore. > propertySizesRelativeToFontSizeOfControlTest > propertySizesRelativeToFontSizeOfParentTest > - Other 8 tests are sanity tests which pass both before and after this fix. Marked as reviewed by dgrieve (Reviewer). - PR: https://git.openjdk.java.net/jfx/pull/397
Re: RFR: 8252935: Add treeShowing listener only when needed [v4]
On Sat, 6 Feb 2021 18:17:40 GMT, John Hendrikx wrote:
>> modules/javafx.graphics/src/main/java/com/sun/javafx/scene/TreeShowingExpression.java
>> line 89:
>>
>>> 87:
>>> NodeHelper.treeVisibleProperty(node).addListener(windowShowingChangedListener);
>>> 88:
>>> 89: nodeSceneChangedListener.changed(null, null, node.getScene());
>>> // registers listeners on Window/Showing
>>
>> We do not follow this pattern of calling the changed() or invalidated()
>> methods. Instead we directly add the necessary calls at the time of
>> initialisation and disposal. I think the pattern should be followed here too.
>> For reference, the pattern is followed in all Skin classes. For example
>> [here](https://github.com/openjdk/jfx/blob/887a443693a04d29ffaff8b8da353db839a987b4/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java#L148)
>> in ButtonSkin, we do not make a call to `sceneChangeListener.changed()`,
>> instead the necessary calls are directly made on [next
>> lines](https://github.com/openjdk/jfx/blob/887a443693a04d29ffaff8b8da353db839a987b4/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java#L151)
>> and then
>> [opposite](https://github.com/openjdk/jfx/blob/887a443693a04d29ffaff8b8da353db839a987b4/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java#L179)
>> calls in dispose().
>>
>> For this line 89 needs to be replaced by following code, (with additional
>> null checks)
>> node.getScene().windowProperty().addListener(sceneWindowChangedListener);
>> node.getScene().getWindow().showingProperty().addListener(windowShowingChangedListener);
>> updateTreeShowing();
>>
>> and similarly they should be removed in dispose().
>
> Isn't it quite error prone to repeat this logic again (especially with all
> the null cases), not to mention that you would need to test the code for the
> initial case (with/without Scene, with/without Window), the "in use" case and
> again for the disposal case?
>
> I personally use helpers for this kind of property chaining as it is far to
> easy to get wrong:
>
> public Binding isShowing(Node node) {
> Values.of(node.sceneProperty())
>.flatMap(s -> Values.of(s.windowProperty()))
>.flatMap(w -> Values.of(w.showingProperty()))
>.orElse(false)
>.toBinding();
> }
>
> The implementation here takes care of `null` and automatically tracks
> property changes and is type safe to boot. I think JavaFX in general could
> really benefit from this, as I've seen this chaining logic repeated a lot.
My concern is about having a similar way of doing something. It would keep the
code uniform. We have been following the earlier pattern under a cleanup task
[JDK-8241364](https://bugs.openjdk.java.net/browse/JDK-8241364). Several bugs
under this task are being fixed in earlier way.
May be we can discuss the new way of handling properties under a separate issue
and plan to modify all such instances at once. Does that sound ok ?
-
PR: https://git.openjdk.java.net/jfx/pull/185
RFR: 8204568: Relative CSS-Attributes don't work all time
Issue is that the size of properties that are relatively(`em`) sized is not computed correctly when the reference `-fx-font-size` is also specified relatively and is nested. Fix is a slight variation of an earlier suggestion in [the PR](https://github.com/javafxports/openjdk-jfx/pull/94). Fix is very specific to this scenario and did not show any side effect nor any test failure. There are 12 new unit tests added along with fix: - Two tests fail before and pass after this fix. These test verify the reported failing scenario. sameRelativeFontSizeNestedParentTest relativeFontSizeDeepNestedParentControlTest - Two other tests fail both before and after this fix. They are not related to the fix. These two tests are ignored. I shall file new JBS issues to track these cases and update PR with the IDs added to @Ignore. propertySizesRelativeToFontSizeOfControlTest propertySizesRelativeToFontSizeOfParentTest - Other 8 tests are sanity tests which pass both before and after this fix. - Commit messages: - fix and test Changes: https://git.openjdk.java.net/jfx/pull/397/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=397=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8204568 Stats: 319 lines in 2 files changed: 317 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jfx/pull/397.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/397/head:pull/397 PR: https://git.openjdk.java.net/jfx/pull/397
Re: RFR: 8259046: ViewPainter.ROOT_PATHS holds reference to Scene causing memory leak
On Wed, 27 Jan 2021 13:31:45 GMT, Kevin Rushforth wrote: > Prism implements a dirty region optimization, where in many cases, only part > of the scene graph is re-rendered when something changes. In support of this, > the `ViewPainter` class in the Quantum Toolkit keeps an array of node paths, > `ROOT_PATHS`, which is a list of sub-trees in the scene graph that need to be > rendered based on a change to that node. The entries in the `ROOT_PATHS` > array are intended to be transient during the rendering of a single pass of a > single scene. They are recreated every time a scene is rendered. The leak > occurs because the entries are not cleared after being used. The fix is to > clear each entry after it is rendered. In addition to static analysis, which > shows that the entries are never used again after a frame is rendered, I have > done a full build and test, including manual tests, to be sure that there is > no regression. > > I have added a test that will fail consistently on Mac and Windows (and > intermittently on Linux) without the fix. It passes consistently on all > platforms with the fix. > > Even though this is a simple change, it is in an area that has historically > been fragile, so I would like two reviewers. I did ran full test and did some run some manual tests also. The changes look fine to me. - Marked as reviewed by pbansal (Committer). PR: https://git.openjdk.java.net/jfx/pull/388
