-
Notifications
You must be signed in to change notification settings - Fork 549
8372761: Prevent degenerate transforms for zero-size Scene/SubScene #1992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
/reviewers 2 |
Webrevs
|
|
I took a quick look and it seems reasonable. I'll let others formally review it, though. Reviewers: @arapte @lukostyra /reviewers 2 |
|
@kevinrushforth |
| assertEquals(minX, b.getMinX(), 0.0005); | ||
| assertEquals(minY, b.getMinY(), 0.0005); | ||
| assertEquals(width, b.getWidth(), 0.0005); | ||
| assertEquals(height, b.getHeight(), 0.0005); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we could declare a static constant EPSILON and use it at least in the modified tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there are many different epsilons used in this test class, I'd have to call it something specific like EPSILON_0005, and at that point, there's little difference between a numeric literal and a constant field. It's a bit like saying "ONE" instead of "1".
| // transform pipeline from being poisoned by NaN or Infinity. For this purpose, we clamp the view width | ||
| // and height to ulp(1), guaranteeing non-zero view dimensions while keeping the clamp very small. | ||
| // | ||
| this.viewWidth = Math.max(Math.ulp(1.0), width); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor suggestion: create a private static function (safeSize ?) with the comment explaining why.
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good on macOS.
|
@lukostyra could you please take a look at this on Windows with fractional scale? |
When a
SceneorSubScenehas its width or height set to 0, coordinate transformations stop working because the transformation matrix contains NaNs. This is a consequence of divisions by zero in both parallel and perspective projections.Even though a 0x0 scene is mathematically degenerate, it is still desirable for coordinate transformation APIs to remain numerically well-behaved and return finite results whenever possible, rather than being poisoned by NaN/Infinity.
Here is an application that demonstrates the failing coordinate transformations. Click on the buttons to resize the SubScene and observe the console output. After applying the patch in this PR, you should observe that
localToScreen()no longer returns NaN.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1992/head:pull/1992$ git checkout pull/1992Update a local copy of the PR:
$ git checkout pull/1992$ git pull https://git.openjdk.org/jfx.git pull/1992/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1992View PR using the GUI difftool:
$ git pr show -t 1992Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1992.diff
Using Webrev
Link to Webrev Comment