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.

Sign up for GitHub

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

Jump to bottom

Refactor CanvasRenderingContext2D.arc() and .ellipse() #25801

Merged
merged 1 commit into from
Feb 26, 2020
Merged

Refactor CanvasRenderingContext2D.arc() and .ellipse() #25801

merged 1 commit into from
Feb 26, 2020

Conversation

pylbrecht
Copy link
Sponsor Contributor

@pylbrecht pylbrecht commented Feb 19, 2020 •
edited

Refactor arc() and ellipse() to make use of lyon_geom::Arc for approximating an arc with quadratic bezier curves.

  • There are tests for these changes

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 19, 2020
@pylbrecht
Copy link
Sponsor Contributor Author

These changes already fix some tests, but I'm trying to squeeze a few more passes out of it. So this is still WIP.

@pylbrecht
Copy link
Sponsor Contributor Author

pylbrecht commented Feb 19, 2020 •
edited

There are still some tests, I'd like to take a look at:

tests/wpt/metadata/2dcontext/path-objects/2d.path.arc.scale.2.html.ini
tests/wpt/metadata/2dcontext/path-objects/2d.path.arc.selfintersect.1.html.ini
tests/wpt/metadata/2dcontext/path-objects/2d.path.arc.selfintersect.2.html.ini
tests/wpt/metadata/2dcontext/path-objects/2d.path.arc.shape.3.html.ini
tests/wpt/metadata/2dcontext/path-objects/2d.path.arc.shape.4.html.ini
tests/wpt/metadata/2dcontext/path-objects/2d.path.arc.twopie.1.html.ini
tests/wpt/metadata/2dcontext/path-objects/2d.path.arc.twopie.3.html.ini

That's all there is left related to arc() (plus the offscreen-canvas equivalents), so I'm trying to make these pass as well and we're done with arc().

@pylbrecht
Copy link
Sponsor Contributor Author

pylbrecht commented Feb 21, 2020 •
edited

These tests depend on jrmuizel/raqote#96 being fixed:

tests/wpt/metadata/2dcontext/path-objects/2d.path.arc.scale.2.html.ini
tests/wpt/metadata/2dcontext/path-objects/2d.path.arc.selfintersect.1.html.ini
tests/wpt/metadata/2dcontext/path-objects/2d.path.arc.selfintersect.2.html.ini
tests/wpt/metadata/2dcontext/path-objects/2d.path.arc.shape.3.html.ini
tests/wpt/metadata/2dcontext/path-objects/2d.path.arc.shape.4.html.ini

This leaves only these two tests for now:

tests/wpt/metadata/2dcontext/path-objects/2d.path.arc.twopie.1.html.ini
tests/wpt/metadata/2dcontext/path-objects/2d.path.arc.twopie.3.html.ini

I'll make those pass and then I would leave it at that for this PR.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 21, 2020
@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Feb 23, 2020
@pylbrecht
Copy link
Sponsor Contributor Author

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

⌛ Trying commit fbbbc87 with merge 14c1e1d...

bors-servo pushed a commit that referenced this pull request Feb 23, 2020
Auto merge of #25801 - pylbrecht:arc.refactor, r=<try>
14c1e1d 
Refactor CanvasRenderingContext2D.arc() and .ellipse()

<!-- Please describe your changes on the following line: -->
Refactor `arc()` and `ellipse()` to make use of `lyon_geom::Arc` for approximating an arc with quadratic bezier curves.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #25331

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@pylbrecht
Copy link
Sponsor Contributor Author

pylbrecht commented Feb 23, 2020 •
edited

@jdm, when all tests pass and you've approved the changes, I'd like to sqash the commits before this will be merged.

@pylbrecht
Copy link
Sponsor Contributor Author

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

⌛ Trying commit 436f8ee with merge ff00e5f...

bors-servo pushed a commit that referenced this pull request Feb 23, 2020
Auto merge of #25801 - pylbrecht:arc.refactor, r=<try>
ff00e5f 
Refactor CanvasRenderingContext2D.arc() and .ellipse()

<!-- Please describe your changes on the following line: -->
Refactor `arc()` and `ellipse()` to make use of `lyon_geom::Arc` for approximating an arc with quadratic bezier curves.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #25331

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

💔 Test failed - status-taskcluster

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label Feb 23, 2020
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Feb 24, 2020
@pylbrecht
Copy link
Sponsor Contributor Author 

4 unexpected results that are NOT known-intermittents:
  ▶ Unexpected subtest result in /offscreen-canvas/line-styles/2d.line.cap.round.html:
  └ PASS [expected FAIL] lineCap 'round' is rendered correctly
  ▶ Unexpected subtest result in /offscreen-canvas/line-styles/2d.line.join.round.html:
  │ FAIL [expected PASS] lineJoin 'round' is rendered correctly
  │   → assert_equals: Red channel of the pixel at (86, 14) expected 0 but got 255
  │ 
  │ _assertPixel@http://web-platform.test:8000/2dcontext/resources/canvas-tests.js:38:5
  │ @http://web-platform.test:8000/offscreen-canvas/line-styles/2d.line.join.round.html:59:1
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1867:25
  └ @http://web-platform.test:8000/offscreen-canvas/line-styles/2d.line.join.round.html:18:3
  ▶ Unexpected subtest result in /offscreen-canvas/line-styles/2d.line.cap.round.worker.html:
  └ PASS [expected FAIL] lineCap 'round' is rendered correctly
  ▶ Unexpected subtest result in /offscreen-canvas/line-styles/2d.line.join.round.worker.html:
  │ FAIL [expected PASS] lineJoin 'round' is rendered correctly
  │   → assert_equals: Red channel of the pixel at (86, 14) expected 0 but got 255
  │ 
  │ _assertPixel@http://web-platform.test:8000/2dcontext/resources/canvas-tests.js:38:5
  │ @http://web-platform.test:8000/offscreen-canvas/line-styles/2d.line.join.round.worker.js:55:1
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1867:25
  └ @http://web-platform.test:8000/offscreen-canvas/line-styles/2d.line.join.round.worker.js:14:3

I'm not exactly sure what happened with these tests, yet. Although they make use of arc(), I can't tell why my changes affect those tests. I'll investigate this.

@jdm jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 24, 2020
@pylbrecht
Copy link
Sponsor Contributor Author

arc()s current implementation is flawed. Instead of drawing a full circle for start = 0; end = 2*PI, it draws nothing. I'm working on it.

@highfive highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Feb 25, 2020
@pylbrecht
Copy link
Sponsor Contributor Author

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

⌛ Trying commit 636f0ed with merge bd544b2...

bors-servo pushed a commit that referenced this pull request Feb 25, 2020
Auto merge of #25801 - pylbrecht:arc.refactor, r=<try>
bd544b2 
Refactor CanvasRenderingContext2D.arc() and .ellipse()

<!-- Please describe your changes on the following line: -->
Refactor `arc()` and `ellipse()` to make use of `lyon_geom::Arc` for approximating an arc with quadratic bezier curves.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #25331

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@pylbrecht
Copy link
Sponsor Contributor Author

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

⌛ Trying commit 7fa03bc with merge 56e750c...

bors-servo pushed a commit that referenced this pull request Feb 25, 2020
Auto merge of #25801 - pylbrecht:arc.refactor, r=<try>
56e750c 
Refactor CanvasRenderingContext2D.arc() and .ellipse()

<!-- Please describe your changes on the following line: -->
Refactor `arc()` and `ellipse()` to make use of `lyon_geom::Arc` for approximating an arc with quadratic bezier curves.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #25331

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@pylbrecht
Copy link
Sponsor Contributor Author

I have a feeling that there may be a more elegant way of calculating the sweep. The checks for various edge cases seem like they could be reduced with some math tricks, but maybe I'm just overthinking.

@bors-servo bors-servo mentioned this pull request Feb 25, 2020
Merged
5 tasks
@bors-servo
Copy link
Contributor

☀️ Test successful - status-taskcluster
State: approved= try=True

@pylbrecht
Copy link
Sponsor Contributor Author

I sqashed the commits, so this is good to go from my side!

@jdm
Copy link
Member

jdm commented Feb 26, 2020

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 86ad6ed has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 26, 2020
@bors-servo
Copy link
Contributor

⌛ Testing commit 86ad6ed with merge ad9bfc2...

@bors-servo
Copy link
Contributor

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing ad9bfc2 to master...

@bors-servo bors-servo merged commit ad9bfc2 into servo:master Feb 26, 2020
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdm jdm

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@pylbrecht @bors-servo @jdm @highfive