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

Better Preserve3D support #1208

Merged
merged 3 commits into from May 8, 2017
Merged

Better Preserve3D support #1208

merged 3 commits into from May 8, 2017

Conversation

@kvark
Copy link
Member

kvark commented May 5, 2017

Follow up to #1169
Includes #1207

r? @glennw @mrobinson

Fixes

  • "preserve-3d" only affects children stacking contexts and contained items
  • generated polygon coordinates of non-zero local bounds
  • sorting order

Servo

The Servo PR is being worked on, but it's not required here, since it may safely continue using TransformStyle::Flat.

Future work

There is at least one feature on the horizon to be implemented before #739 can be truly closed.
The TransformStyle should be moved out of the stacking context and deserve it's own pushable item (similar to clip-scroll groups). This is required because an item without "preserve-3d" automatically becomes "flat" but does not establish a stacking context, which is tested by Servo.

Edit: apparently, Chrome disagrees here, so the current approach of WR might stay.
Create Bugstar issue-1362543.


This change is Reviewable

@kvark kvark force-pushed the kvark:preserve-3d branch from dd54af3 to ef11311 May 5, 2017
@glennw
Copy link
Member

glennw commented May 7, 2017

Reviewed 24 of 24 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


webrender/src/frame_builder.rs, line 90 at r1 (raw file):

fn make_polygon(sc: &StackingContext, node: &ClipScrollNode, anchor: usize)
                -> Polygon<f32, WorldPixel> {
    //TODO: only work with `sc.local_bounds` worth of space

Could you expand on this comment a bit, and why local_bounds.origin is not used?


webrender/src/frame_builder.rs, line 92 at r1 (raw file):

    //TODO: only work with `sc.local_bounds` worth of space
    let size = sc.local_bounds.bottom_right();
    let bounds = LayerRect::new(sc.reference_frame_offset, LayerSize::new(size.x, size.y));

Is size already a LayerSize?


wrench/reftests/split/reftest.list, line 3 at r1 (raw file):

== simple.yaml simple-ref.yaml
== order.yaml order-ref.yaml
#TODO: == nested.yaml nested-ref.yaml

Is this meant to be resolved before merge, or as a follow up?


Comments from Reviewable

@glennw
Copy link
Member

glennw commented May 7, 2017

@kvark Looks good - just a couple minor issues.

@kvark kvark force-pushed the kvark:preserve-3d branch from ef11311 to 3262ea1 May 8, 2017
@kvark
Copy link
Member Author

kvark commented May 8, 2017

@glennw thanks for the review! Your concerns are hopefully addressed now. Please take a look at the last commit.


Review status: 11 of 21 files reviewed at latest revision, 3 unresolved discussions.


webrender/src/frame_builder.rs, line 90 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Could you expand on this comment a bit, and why local_bounds.origin is not used?

good idea!


webrender/src/frame_builder.rs, line 92 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Is size already a LayerSize?

unfortunately, it's LayerPoint, and euclid doesn't seem to provide convenience conversion between these


wrench/reftests/split/reftest.list, line 3 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Is this meant to be resolved before merge, or as a follow up?

nicely spotted! I worked on that test and encountered a logical problem with the code, related to handling the order of planes that geometrically match each other.


Comments from Reviewable

@glennw
Copy link
Member

glennw commented May 8, 2017

@bors-servo
Copy link
Contributor

bors-servo commented May 8, 2017

📌 Commit 3262ea1 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented May 8, 2017

Testing commit 3262ea1 with merge def8647...

bors-servo added a commit that referenced this pull request May 8, 2017
Better Preserve3D support

Follow up to #1169
Includes #1207

r? @glennw @mrobinson

  - "preserve-3d" only affects children stacking contexts and contained items
  - generated polygon coordinates of non-zero local bounds
  - sorting order

The Servo PR is being [worked on](servo/servo@master...kvark:preserve3d), but it's not required here, since it may safely continue using `TransformStyle::Flat`.

There is at least one feature on the horizon to be implemented before  #739 can be truly closed.
The `TransformStyle` should be moved out of the stacking context and deserve it's own pushable item (similar to clip-scroll groups). This is required because an item without "preserve-3d" automatically becomes "flat" but does not establish a stacking context, which is [tested by Servo](https://raw.githubusercontent.com/servo/servo/master/tests/wpt/css-tests/css-transforms-1_dev/html/transform3d-sorting-004.htm).

Edit: apparently, Chrome disagrees here, so the current approach of WR might stay.
Create Bugstar [issue-1362543](https://bugzilla.mozilla.org/show_bug.cgi?id=1362543).

<!-- 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/1208)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 8, 2017

☀️ Test successful - status-travis
Approved by: glennw
Pushing def8647 to master...

@bors-servo bors-servo merged commit 3262ea1 into servo:master May 8, 2017
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 10 files, 3 discussions left (glennw)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@kvark kvark mentioned this pull request May 8, 2017
3 of 5 tasks complete
@kvark kvark deleted the kvark:preserve-3d branch May 8, 2017
bors-servo added a commit to servo/servo that referenced this pull request May 14, 2017
WR update: preserve-3d support

<!-- Please describe your changes on the following line: -->

This is WR update to servo/webrender@d335555 having new features:
  - limited "preserve-3d" support (servo/webrender#1169, servo/webrender#1208)
  - rayon thread pool (servo/webrender#1202)
  - further border rendering improvements

Edit: the references to bincode serialization and border styles are removed from here, since they are already integrated into Servo.
Edit2: this is alternative/similar to  #16801, based on @mrobinson code (see first commit).

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

Related to #9087
Note that I'm unlocking a few tests as well as changing some related to `preserve-3d`. The changes come from common sense and comparison to Chromium. I'm ready to discuss them on a individual basis.

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

There is still an investigation to do with regards to the differences of preserve3d logic between Blink and Gecko.

<!-- 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. -->

<!-- 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/16775)
<!-- Reviewable:end -->
@staktrace
Copy link
Contributor

staktrace commented May 19, 2017

Edit: apparently, Chrome disagrees here, so the current approach of WR might stay.
Create Bugstar issue-1362543.

@kvark, from the discussion on bug 1362543, it looks like Chrome was correct? Are there any other WR changes needed for preserve-3d, or can we close out #739?

@kvark
Copy link
Member Author

kvark commented May 19, 2017

@staktrace I want to get some real use cases going (outside of our artificial tests) before closing the issue.

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.