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

Improve rendering of transformed 2d paths #21310

Merged
merged 6 commits into from Feb 11, 2019

Conversation

Projects
None yet
5 participants
@jdm
Copy link
Member

jdm commented Aug 1, 2018

This makes http://webglreport.com/ render correctly by porting the equivalent code from Gecko's 2d canvas implementation that handles paths with transformations. The first commit fixes the rendering issue, and the second commit fixes some new test failures that were exposed by that change.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #21169
  • There are tests for these changes

This change is Reviewable

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Aug 1, 2018

r? @pcwalton
Patrick, this seems like it's in your wheelhouse, even if you haven't looked at the 2d canvas code much before.

@highfive highfive assigned pcwalton and unassigned nox Aug 1, 2018

@@ -183,36 +185,50 @@ impl<'a> CanvasData<'a> {
}

pub fn begin_path(&mut self) {
self.path_builder = self.drawtarget.create_path_builder()
self.path_builder = self.drawtarget.create_path_builder();
self.path = None;

This comment has been minimized.

@pcwalton

pcwalton Aug 1, 2018

Contributor

Shouldn't all functions that touch the path builder also invalidate this cached Path?

Maybe we should have an accessor for &mut path_builder that first invalidates the cached Path, and redirect all direct mutable access to self.path_builder through that.

/// User-space path builder.
path_builder: Option<PathBuilder>,
/// Device-space path builder, if transforms are added during path building.
device_space_path_builder: Option<PathBuilder>,

This comment has been minimized.

@pcwalton

pcwalton Aug 1, 2018

Contributor

Why do we need to store the user-space path? Could we just store the path in device space?

@jdm jdm force-pushed the jdm:canvas-path branch from e2a77aa to c343ef7 Aug 7, 2018

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Aug 7, 2018

Comments addressed and the resulting state machine is much nicer to reason about than the collection of members.

@jdm jdm force-pushed the jdm:canvas-path branch from c343ef7 to d6057d8 Aug 7, 2018

Show resolved Hide resolved components/canvas/canvas_data.rs Outdated
Some(PathState::UserSpacePathBuilder(_, ref mut transform)) |
Some(PathState::UserSpacePath(_, ref mut transform)) => {
if transform.is_none() {
*transform = Some(self.drawtarget.get_transform());

This comment has been minimized.

@pcwalton

pcwalton Aug 7, 2018

Contributor

So if you call set_transform twice in a row, only the first one will take effect on the PathState (because on the second one, the transform will not be None anymore). Is that correct behavior?

This comment has been minimized.

@jdm

jdm Feb 7, 2019

Author Member

This is correct. The transform that is stored is the transform that applies to all existing points on the path and needs to be applied to those points when creating the device space path builder.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Nov 7, 2018

☔️ The latest upstream changes (presumably #22126) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm jdm force-pushed the jdm:canvas-path branch from d6057d8 to 2c5a6eb Feb 7, 2019

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Feb 7, 2019

All review comments addressed.

@jdm jdm removed the S-needs-rebase label Feb 7, 2019

@pcwalton

This comment has been minimized.

Copy link
Contributor

pcwalton commented Feb 11, 2019

r=me. Looks good!

@jdm

This comment has been minimized.

Copy link
Member Author

jdm commented Feb 11, 2019

@bors-servo r=pcwalton

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Feb 11, 2019

📌 Commit 18282b8 has been approved by pcwalton

bors-servo added a commit that referenced this pull request Feb 11, 2019

Auto merge of #21310 - jdm:canvas-path, r=pcwalton
Improve rendering of transformed 2d paths

This makes http://webglreport.com/ render correctly by porting the [equivalent code](https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/dom/canvas/CanvasRenderingContext2D.h#847-875) from Gecko's 2d canvas implementation that handles paths with transformations. The first commit fixes the rendering issue, and the second commit fixes some new test failures that were exposed by that change.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21169
- [x] There are tests for these changes

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Feb 11, 2019

⌛️ Testing commit 18282b8 with merge 3d4a416...

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Feb 11, 2019

@bors-servo bors-servo merged commit 18282b8 into servo:master Feb 11, 2019

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.