Re: RFR: 8202990: javafx webview css filter property with display scaling [v2]
On Tue, 1 Sep 2020 06:29:17 GMT, Bhawesh Choudhary
wrote:
>> ImageJava.cpp ignores CompositeOperator parameter in drawImage function due
>> to which shadow was getting drawn on top of
>> actual image. apply given composite operator to graphics context before
>> drawing image to fix this issue. another issue
>> is into WCGraphicsPrismContext.java. while blending two layers, applying
>> state to the destination layer was missed due
>> to which image was not getting drawn with right scale in hidpi mode. apply
>> state to fix the issue.
>
> Bhawesh Choudhary has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Moved test from unit test to system test
tests/system/src/test/java/test/javafx/scene/web/CSSFilterTest.java line 93:
> 92: double deltaOpacity = Math.abs(notExpected.getOpacity() -
> actual.getOpacity());
> 93: if (deltaRed < delta && deltaGreen < delta && deltaBlue < delta
> && deltaOpacity < delta) {
> 94: fail(msg + " not expected:" + colorToString(notExpected)
testColorEquals can be reused here.
`if (testColorEquals(expected, actual, delta))`
-
PR: https://git.openjdk.java.net/jfx/pull/279
Re: RFR: 8202990: javafx webview css filter property with display scaling [v2]
On Tue, 1 Sep 2020 06:29:17 GMT, Bhawesh Choudhary wrote: >> ImageJava.cpp ignores CompositeOperator parameter in drawImage function due >> to which shadow was getting drawn on top of >> actual image. apply given composite operator to graphics context before >> drawing image to fix this issue. another issue >> is into WCGraphicsPrismContext.java. while blending two layers, applying >> state to the destination layer was missed due >> to which image was not getting drawn with right scale in hidpi mode. apply >> state to fix the issue. > > Bhawesh Choudhary has updated the pull request incrementally with one > additional commit since the last revision: > > Moved test from unit test to system test tests/system/src/test/java/test/javafx/scene/web/CSSFilterTest.java line 44: > 43: import static javafx.concurrent.Worker.State.SUCCEEDED; > 44: import static org.junit.Assert.assertEquals; > 45: import static org.junit.Assert.assertNotNull; assertEquals is not used in this test and can be removed. - PR: https://git.openjdk.java.net/jfx/pull/279
Re: RFR: 8217472: Add attenuation for PointLight [v5]
On Tue, 1 Sep 2020 22:46:44 GMT, Nir Lisker wrote: >> You need to pass a flag to enable system tests (and a second one for Robot >> tests, but if you use snapshot it might not >> need to be). >> gradle -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests MyTest > >> gradle -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests MyTest > > What format is `MyTest`? Is it some relative path? It can either be a fully qualified class name (with `.` as separator) or the unqualified name of the test class. The class name must end with exactly `Test`. So for example, try it with `Snapshot1Test`. - PR: https://git.openjdk.java.net/jfx/pull/43
Re: RFR: 8217472: Add attenuation for PointLight [v5]
On Tue, 1 Sep 2020 17:00:10 GMT, Kevin Rushforth wrote: > gradle -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests MyTest What format is `MyTest`? Is it some relative path? - PR: https://git.openjdk.java.net/jfx/pull/43
Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView [v3]
On Tue, 1 Sep 2020 14:12:15 GMT, Jeanette Winzenburg wrote: >> Since there was a problem that the cell was not updated when the window was >> maximized, I added a fix to >> TreeTableViewSkin.java. >> Added test code (BigTreeTableViewTest) for TreeTableView to the first post. >> >> @kleopatra >> Does the test code I added (BigTreeTableViewTest.java) also have side >> effects? > >> >> >> Since there was a problem that the cell was not updated when the window was >> maximized, I added a fix to >> TreeTableViewSkin.java. > > that's just the added listener to hbar properties (same as in TableViewSkin), > or anything else changed? > >> Added test code (BigTreeTableViewTest) for TreeTableView to the first post. >> >> @kleopatra >> Does the test code I added (BigTreeTableViewTest.java) also have side >> effects? > > yeah, but why do you ask me - you could easily see for yourself :) And don't > understand why you wrap the hierarchy > setup in runlater (which smears a bit over the delay by filling the treeTable > content at an unspecific time "later") When the startup time is measured by eye, the impression changes depending on the individual difference. The effect of runLater affects your experience. However, I succeeded in further improving performance by eliminating another bottleneck in TreeTableView. Of course, it also includes improvements in startup time. I plan to commit at a later date. - PR: https://git.openjdk.java.net/jfx/pull/125
Re: RFR: 8202990: javafx webview css filter property with display scaling [v2]
On Tue, 1 Sep 2020 06:29:17 GMT, Bhawesh Choudhary wrote: >> ImageJava.cpp ignores CompositeOperator parameter in drawImage function due >> to which shadow was getting drawn on top of >> actual image. apply given composite operator to graphics context before >> drawing image to fix this issue. another issue >> is into WCGraphicsPrismContext.java. while blending two layers, applying >> state to the destination layer was missed due >> to which image was not getting drawn with right scale in hidpi mode. apply >> state to fix the issue. > > Bhawesh Choudhary has updated the pull request incrementally with one > additional commit since the last revision: > > Moved test from unit test to system test Looks good. I verified that the system test still catches the bug (that is, it fails without the fix and passes with the fix). I ran it in a loop 100 times on both Mac and Windows without crashing. - Marked as reviewed by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/279
Re: RFR: 8217472: Add attenuation for PointLight [v5]
On Tue, 1 Sep 2020 13:49:48 GMT, Nir Lisker wrote: >> I guess you are trying a unit test in the `javafx.graphics` project? That >> won't work. It needs to be a system test in >> `tests/system/...` > > I tried several approaches. When I put it under `tests/system` and run the > tests task, I don't see it being run. I'm > not familiar with the way tests under `tests` are run in general. You need to pass a flag to enable system tests (and a second one for Robot tests, but if you use snapshot it might not need to be). gradle -PFULL_TEST=true -PUSE_ROBOT=true :systemTests:test --tests MyTest - PR: https://git.openjdk.java.net/jfx/pull/43
Re: RFR: 8185886: Improve scrolling performance of TableView and TreeTableView [v3]
On Mon, 31 Aug 2020 19:16:31 GMT, yosbits wrote: > > > Since there was a problem that the cell was not updated when the window was > maximized, I added a fix to > TreeTableViewSkin.java. that's just the added listener to hbar properties (same as in TableViewSkin), or anything else changed? > Added test code (BigTreeTableViewTest) for TreeTableView to the first post. > > @kleopatra > Does the test code I added (BigTreeTableViewTest.java) also have side effects? yeah, but why do you ask me - you could easily see for yourself :) And don't understand why you wrap the hierarchy setup in runlater (which smears a bit over the delay by filling the treeTable content at an unspecific time "later") - PR: https://git.openjdk.java.net/jfx/pull/125
Re: RFR: 8209788: Left/Right/Ctrl+A keys not working in editor of ComboBox if popup showing [v8]
On Sat, 29 Aug 2020 10:40:35 GMT, Jeanette Winzenburg wrote: > What do you think? Suggestion looks promising, I shall try it and update. - PR: https://git.openjdk.java.net/jfx/pull/172
Re: RFR: 8217472: Add attenuation for PointLight [v5]
On Tue, 1 Sep 2020 11:42:30 GMT, Kevin Rushforth wrote:
>> I've written a simple draft for the automated test:
>>
>> PointLight light = new PointLight(Color.BLUE);
>> light.setTranslateZ(-50);
>> var box = new Box(100, 100, 1);
>> var root = new Group(light, box);
>> var scene = new Scene(root);
>> var image = box.snapshot(null, null);
>> var nonAttenColor = image.getPixelReader().getColor(1, 1);
>> light.setLinearAttenuation(2);
>> image = box.snapshot(null, null);
>> var attenColor = image.getPixelReader().getColor(1, 1);
>> assertTrue("Attenuation color should be less than non-attenuated",
>> nonAttenColor.getBlue() > attenColor.getBlue());
>>
>> However, I'm getting the error:
>>
>> java.lang.UnsupportedOperationException
>> at
>> javafx.graphics/test.com.sun.javafx.pgstub.StubToolkit.renderToImage(StubToolkit.java:719)
>> at javafx.graphics/javafx.scene.Scene.doSnapshotTile(Scene.java:1383)
>> at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1319)
>> at javafx.graphics/javafx.scene.Node.doSnapshot(Node.java:2136)
>> at javafx.graphics/javafx.scene.Node.snapshot(Node.java:2214)
>> Where is the test that takes the snapshot? I am missing something in my
>> setup.
>
> I guess you are trying a unit test in the `javafx.graphics` project? That
> won't work. It needs to be a system test in
> `tests/system/...`
I tried several approaches. When I put it under `tests/system` and run the
tests task, I don't see it being run. I'm
not familiar with the way tests under `tests` are run in general.
-
PR: https://git.openjdk.java.net/jfx/pull/43
Re: RFR: 8217472: Add attenuation for PointLight [v5]
On Tue, 1 Sep 2020 10:54:50 GMT, Nir Lisker wrote:
>> The performance tests need a standard GPLv2+classpath copyright header. I
>> haven't tested them yet, but will do that
>> next week.
>
> I've written a simple draft for the automated test:
>
> PointLight light = new PointLight(Color.BLUE);
> light.setTranslateZ(-50);
> var box = new Box(100, 100, 1);
> var root = new Group(light, box);
> var scene = new Scene(root);
> var image = box.snapshot(null, null);
> var nonAttenColor = image.getPixelReader().getColor(1, 1);
> light.setLinearAttenuation(2);
> image = box.snapshot(null, null);
> var attenColor = image.getPixelReader().getColor(1, 1);
> assertTrue("Attenuation color should be less than non-attenuated",
> nonAttenColor.getBlue() > attenColor.getBlue());
>
> However, I'm getting the error:
>
> java.lang.UnsupportedOperationException
> at
> javafx.graphics/test.com.sun.javafx.pgstub.StubToolkit.renderToImage(StubToolkit.java:719)
> at javafx.graphics/javafx.scene.Scene.doSnapshotTile(Scene.java:1383)
> at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1319)
> at javafx.graphics/javafx.scene.Node.doSnapshot(Node.java:2136)
> at javafx.graphics/javafx.scene.Node.snapshot(Node.java:2214)
> Where is the test that takes the snapshot? I am missing something in my setup.
I guess you are trying a unit test in the `javafx.graphics` project? That won't
work. It needs to be a system test in
`tests/system/...`
-
PR: https://git.openjdk.java.net/jfx/pull/43
Re: RFR: 8218973: SVG with masking is not rendering image with mask effect [v10]
> Root cause of issue is Specifying a image mask from GraphicsContextJava.cpp > in WebKit was not implemented, so masking > doesn't take place at all while rendering SVGRect. to fix this issue add > implementation of function clipToImageBuffer() > in GraphicsContextJava.cpp and send clip image to WCGraphicsPrismContext.java > While rendering in > WCGraphicsPrismContext.java if image clip mask is available, use it for > rendering using MaskTextureGraphics interface > otherwise use usual way of rendering. Bhawesh Choudhary has updated the pull request incrementally with one additional commit since the last revision: Added fillRoundedRect and fillPath drawing with mask - Changes: - all: https://git.openjdk.java.net/jfx/pull/213/files - new: https://git.openjdk.java.net/jfx/pull/213/files/41f64c0e..87f63074 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=213=09 - incr: https://webrevs.openjdk.java.net/?repo=jfx=213=08-09 Stats: 42 lines in 1 file changed: 42 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/213.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/213/head:pull/213 PR: https://git.openjdk.java.net/jfx/pull/213
Re: RFR: 8217472: Add attenuation for PointLight [v5]
On Fri, 7 Aug 2020 22:37:15 GMT, Kevin Rushforth wrote:
>> Given that we don't have any automated tests for lighting (we have a couple
>> that verify that we can take a snapshot and
>> get the same result, but that isn't testing the lighting itself), probably
>> the most we can expect is a simple test of a
>> large quad with a light fairly close to the object, such that the difference
>> with / without attenuation is noticeable.
>> Then you could sample the center and near the corners of the object for both
>> the attenuated and unattenuated cases.
>
> The performance tests need a standard GPLv2+classpath copyright header. I
> haven't tested them yet, but will do that
> next week.
I've written a simple draft for the automated test:
PointLight light = new PointLight(Color.BLUE);
light.setTranslateZ(-50);
var box = new Box(100, 100, 1);
var root = new Group(light, box);
var scene = new Scene(root);
var image = box.snapshot(null, null);
var nonAttenColor = image.getPixelReader().getColor(1, 1);
light.setLinearAttenuation(2);
image = box.snapshot(null, null);
var attenColor = image.getPixelReader().getColor(1, 1);
assertTrue("Attenuation color should be less than non-attenuated",
nonAttenColor.getBlue() > attenColor.getBlue());
However, I'm getting the error:
java.lang.UnsupportedOperationException
at
javafx.graphics/test.com.sun.javafx.pgstub.StubToolkit.renderToImage(StubToolkit.java:719)
at javafx.graphics/javafx.scene.Scene.doSnapshotTile(Scene.java:1383)
at javafx.graphics/javafx.scene.Scene.doSnapshot(Scene.java:1319)
at javafx.graphics/javafx.scene.Node.doSnapshot(Node.java:2136)
at javafx.graphics/javafx.scene.Node.snapshot(Node.java:2214)
Where is the test that takes the snapshot? I am missing something in my setup.
-
PR: https://git.openjdk.java.net/jfx/pull/43
Re: Possible approaches to JDK-8185886: Improve scrolling performance of TableView and TreeTableView
The "dynamic update performance" performance issue we are seeing is a
regression from JDK-8090322. In this change the Node treeShowing
property was introduced. The JDK-8090322 warns in its description about:
"""Considerations:
* This is too expensive to calculate for all nodes by default. So the
simplest way to provide it would be a special binding implementation or
a util class. Where you create a instance and pass in the node you are
interested in. It can then register listeners all the way up the tree
and listen to what it needs. """
The above comment is warning against the fact that registering listeners
for EACH Node on Window and Scene is going to be a performance issue.
As nodes can number in the 1000's or 10.000's, that's a lot of listeners
to store in a List data structure.
PR 185 targets this issue and implements the feature in JDK-8090322 in
the way that was suggested in the above comment, instead of how it
currently is implemented (registering listeners for every Node, just in
case someone needs the treeShowing property).
It's scope is not as broad as it would seem (because of a change in
Node). It effectively only makes a small change in two controls
(PopupWindow and ProgressIndicatorSkin).
--John
On 31/08/2020 16:54, Jeanette Winzenburg wrote:
Looking at the examples provided in 108/125: apart from both having many
columns (> 300 makes them really nasty) they differ in
Table content:
125 - static data
108 - items are frequently modified (added)
Perceived performance:
125 - vertical scrolling: thumb/content lags behind mouse
108 - with enough columns, all interaction is sluggish (mouse, keys,
..), scrolling being just one of them
Both have examples, running those against the suggested fixes (with 108a
for Jose's approach)
125 - fixes its own example, does nothing for the other
108, 108a, 185 - improves its own example, does nothing for other
So we seem to have multiple issues that are (mostly) orthogonal: one
being the broken/missing horizontal virtualization (125), the other
related to dynamic update of table content (108, 108a, 185). We need to
solve both, the solution/s for one looks (mostly?) unrelated to the
solution to the other.
125 is the only one PR for its use-case, and it seems to do its job.
From those targeting the dynamic data update all except Jose's (not yet
formalized into a PR) approach feel too broad: table's reaction to items
modifications is .. suboptimal in more than one aspect. Changing overall
notification implementation to improve that, sounds like covering it up.
-- Jeanette
Zitat von Kevin Rushforth :
Sorry for the badly formatted html. Here it is again.
I see some progress being made on the {Tree}TableView performance
issue. To summarize where I think we are:
There are currently 2 different approaches under review:
1. https://github.com/openjdk/jfx/pull/108 : optimization in
javafx.base to make removing listeners faster
2. https://github.com/openjdk/jfx/pull/125 : optimization in TableView
to reduce the number of add / removes
In addition, the following is a WIP PR that the author thinks could be
a 3rd approach:
3. https://github.com/openjdk/jfx/pull/185 : optimization in scene
graph to avoid install treeShowing listeners on Window and Scene for
all nodes
Jose has proposed a 4th approach as a comment to PR #108, and as I
understand it, he will propose a PR shortly.
4. Don't clear the list of children in a VirtualFlow when the number
of items changes.
So the first thing that is needed is to evaluate the approaches and
decide which one to pursue.
Options 1 and 3 are more broad in their scope, option #2 is more
targeted (to TableView), but within that area is a larger change.
Option #3 would remove the (internal) treeShowing property as a
generally available concept and only use it for controls like
ProgressIndicator that really need it. Option #4 affects only controls
that use VirtualFlow (ListView, TableVIew, TreeTableView), and seems
not to be a large change (presuming we can verify that no leak is
introduced).
I note that these fixes are not mutually exclusive, but I do think we
need to settle on a primary approach and use that to fix this issue.
If there are still performance problems after that fix, we can
consider one (or more) of the others.
Comments?
-- Kevin
Re: RFR: 8202990: javafx webview css filter property with display scaling [v2]
> ImageJava.cpp ignores CompositeOperator parameter in drawImage function due > to which shadow was getting drawn on top of > actual image. apply given composite operator to graphics context before > drawing image to fix this issue. another issue > is into WCGraphicsPrismContext.java. while blending two layers, applying > state to the destination layer was missed due > to which image was not getting drawn with right scale in hidpi mode. apply > state to fix the issue. Bhawesh Choudhary has updated the pull request incrementally with one additional commit since the last revision: Moved test from unit test to system test - Changes: - all: https://git.openjdk.java.net/jfx/pull/279/files - new: https://git.openjdk.java.net/jfx/pull/279/files/91e8bdf0..a99141df Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=279=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=279=00-01 Stats: 211 lines in 3 files changed: 185 ins; 26 del; 0 mod Patch: https://git.openjdk.java.net/jfx/pull/279.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/279/head:pull/279 PR: https://git.openjdk.java.net/jfx/pull/279
