On Fri, 17 Jan 2020 17:25:51 GMT, Frederic Thevenet <github.com+7450507+ftheve...@openjdk.org> wrote:
>>> >>> >>> I tested this fix against the repro code in >>> [javafxports/openjdk-jfx#433](https://github.com/javafxports/openjdk-jfx/issues/433) >>> (which is >>> [JDK-8222238](https://bugs.openjdk.java.net/browse/JDK-8222238)), but there >>> is still an NPE. I'm not certain that this fix is supposed to solve that >>> bug, but according to the comments, the root cause is the same as >>> [JDK-8189082](https://bugs.openjdk.java.net/browse/JDK-8189082), which is >>> related to this one. It's worth to take a look to see if something was >>> missed. >> >> Although JDK-8189082 (and potentially others) are indeed likely to share the >> same root cause, the fix proposed here won't have an effect on anything else >> than snapshots, since the tiling is done at the Scene level rather than >> within QuantumToolkit. >> I purposefully choose to take the "easy" way out to limit the potential >> side-effects of the change, but I agree that on the other hand supporting >> tiling when needed directly in QuantumToolkit::renderToImage would have the >> potential to solve a whole category of issues. >> Also, from a very pragmatic angle, because of the increased complexity and >> scope and the risks that come with it, I'm not very optimistic that this >> change could realistically make it into jfx14 and to be honest I'm quite >> eager to get the screenshot feature in my app working properly ASAP. >> >> I'd be willing to try and work on a more generic fix under the umbrella of >> JDK-8189082, that'd be targeted at 15, while still having this fix included >> as part 14; but that means this should (ideally) be reverted once it becomes >> useless. >> I don't know if the idea of having a "temporary fix" approach seems >> acceptable in that context. > > Wel,l upon closer inspection it appears `QuantumToolkit::renderToImage` is > only really used by `Scene::doSnapshot` anyway. Rendering of a the content of > an NGSubScene (the underlying issue in JDK-8189082) is handled in a > completely different code path, and I suspect this is also true for Canvas, > where I hear a similar kind of issue exists. > So from my (admittedly limited) understanding, fixing all problems at once > would requires handling the tiling transparently within `RTTexture` (or more > exactly inside all of its GraphicsPipeline specifics implementations) which > seems quite a bit more complicated and risky. > upon closer inspection it appears `QuantumToolkit::renderToImage` is only > really used by `Scene::doSnapshot` anyway. Correct. > So from my (admittedly limited) understanding, fixing all problems at once > would requires handling the tiling transparently within `RTTexture` (or more > exactly inside all of its GraphicsPipeline specifics implementations) which > seems quite a bit more complicated and risky. That would be one approach (extending RTT to handle tiling automatically), but I'm not at all sure that it would be the best one. It would as you note, be more complicated and risky. I have no problem with your current proposed approach. I agree with @arapte that this seems safe for openjfx14. To that end, can you retarget this PR to the `jfx14` branch? Since there are no post-jfx14 commits from master in your local branch, this should be as simple as editing the PR and changing the target. Then we can complete the review, hopefully next week. ------------- PR: https://git.openjdk.java.net/jfx/pull/68
