Skip to content
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

Use one iteration of Newton's method to approximate distance to an ellipse rather than computing the exact distance. #2423

Merged
merged 2 commits into from Feb 16, 2018

Conversation

@pcwalton
Copy link
Collaborator

pcwalton commented Feb 14, 2018

This is a well-known trick. It's described in Loop-Blinn and is
thoroughly detailed in G. Taubin, "Distance Approximations for
Rasterizing Implicit Curves".

I've verified that this looks good for circular borders, elliptical borders, and both under rotation. Perspective is broken, but in the same way it's already broken in master (see #2421).

Let me know if there are any reftests/etc. you'd like me to add.

r? @glennw


This change is Reviewable

@glennw
Copy link
Member

glennw commented Feb 14, 2018

We have a fair number of reftests with ellipse borders, so if this passes those, it should be fine.

We'll need a gecko try though as well.

@glennw
Copy link
Member

glennw commented Feb 14, 2018

@pcwalton OK, the reftests are failing - but it might just be subpixel differences from the reference image. If you run ./script/headless.py reftest >ref.log you'll be able to view the differences in the reftest analyzer (which is in-tree). If you need to re-generate reference images, you can use ./script/headless.py png <yaml file>.

@pcwalton pcwalton force-pushed the pcwalton:ellipse branch from f9eb7e0 to 568e961 Feb 15, 2018
@pcwalton
Copy link
Collaborator Author

pcwalton commented Feb 15, 2018

@glennw OK, I fixed failures relating to zero-width borders and updated the various reftests.

@pcwalton pcwalton force-pushed the pcwalton:ellipse branch from 568e961 to 7bdc799 Feb 15, 2018
@pcwalton
Copy link
Collaborator Author

pcwalton commented Feb 15, 2018

Cropped the reference images. r? @glennw

ellipse rather than computing the exact distance.

This is a well-known trick. It's described in Loop-Blinn and is
thoroughly detailed in G. Taubin, "Distance Approximations for
Rasterizing Implicit Curves".
@glennw glennw force-pushed the pcwalton:ellipse branch from 7bdc799 to 9fb572b Feb 16, 2018
@glennw glennw force-pushed the pcwalton:ellipse branch from 9fb572b to 30f9415 Feb 16, 2018
@glennw
Copy link
Member

glennw commented Feb 16, 2018

Rebased and pushed updated Linux-specific reftest images. Let's see if CI is happy now.

@glennw
Copy link
Member

glennw commented Feb 16, 2018

There are some failures in the gecko try. The R8 and R4 failures are known (#2405). From a quick look at the R3 failures, they seem to be subpixel differences between the SVG rendered blob and the WR rendered version. I think they should all be fine to fuzz - but I'll get @staktrace to confirm before we merge.

@glennw
Copy link
Member

glennw commented Feb 16, 2018

r=me if @staktrace is happy with the try, btw.

@staktrace
Copy link
Contributor

staktrace commented Feb 16, 2018

Sorry for the delay. The R3 failures look fine - those tests are all fuzzy already and the increase in fuzz is just by one in most cases, so I'm fine with that. The R4 one was not fuzzy before as far as I know (I don't see anything about it in #2405?) but the changes are minor and I'm ok with fuzzing it now. R8 failure is known, as @glennw said. So I'm good with this, thanks!

@staktrace
Copy link
Contributor

staktrace commented Feb 16, 2018

The R4 one was not fuzzy before as far as I know

Ignore this bit, #2405 introduced nondeterministic fuzziness, so that confused me for a bit.

@glennw
Copy link
Member

glennw commented Feb 16, 2018

@bors-servo r+ 🚀

@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2018

📌 Commit 30f9415 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2018

Testing commit 30f9415 with merge 2067b37...

bors-servo added a commit that referenced this pull request Feb 16, 2018
Use one iteration of Newton's method to approximate distance to an ellipse rather than computing the exact distance.

This is a well-known trick. It's described in Loop-Blinn and is
thoroughly detailed in G. Taubin, "Distance Approximations for
Rasterizing Implicit Curves".

I've verified that this looks good for circular borders, elliptical borders, and both under rotation. Perspective is broken, but in the same way it's already broken in master (see #2421).

Let me know if there are any reftests/etc. you'd like me to add.

r? @glennw

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2423)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2018

☀️ Test successful - status-appveyor, status-taskcluster, status-travis
Approved by: glennw
Pushing 2067b37 to master...

@bors-servo bors-servo merged commit 30f9415 into servo:master Feb 16, 2018
3 checks passed
3 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@pcwalton pcwalton deleted the pcwalton:ellipse branch Feb 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.