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

spatial-node: Flatten transforms properly. #3401

Merged
merged 3 commits into from Dec 12, 2018

Conversation

Projects
None yet
4 participants
@emilio
Copy link
Member

emilio commented Dec 12, 2018

This fixes Gecko bug 1512537.

Fixes #3394


This change is Reviewable

@emilio emilio force-pushed the emilio:flatten-transform branch from 9140ecc to d5a23f5 Dec 12, 2018

spatial-node: Flatten transforms properly.
This fixes Gecko bug 1512537.

Fixes #3394

@emilio emilio force-pushed the emilio:flatten-transform branch from d5a23f5 to 0cb14df Dec 12, 2018

@emilio

This comment has been minimized.

Copy link
Member

emilio commented Dec 12, 2018

Looks like the taskcluster failure may be a flake:

error: failed to execute compile
caused by: Failed to send data to or receive data from server
caused by: Failed to read response header
caused by: Connection reset by peer (os error 54)
error: Could not compile `osmesa-src`.
@staktrace

This comment has been minimized.

Copy link
Contributor

staktrace commented Dec 12, 2018

Looks like the taskcluster failure may be a flake:

Yeah that's mozilla/sccache#344

@kvark
Copy link
Member

kvark left a comment

Looks good in general, thank you!
I've got a few questions/concerns to resolve.

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @emilio)


webrender/src/spatial_node.rs, line 495 at r1 (raw file):

                let should_flatten =
                    info.transform_style == TransformStyle::Flat &&
                    info.source_perspective.is_identity();

if we do have perspective, shouldn't we flatten anyway (for Flat mode), just with perspective multiplied first?


webrender/src/util.rs, line 573 at r1 (raw file):

impl<Src, Dst> Clone for FastTransform<Src, Dst> {
    fn clone(&self) -> Self {

I don't understand this part. Copy depends on Clone, but you implement Clone as a copy operation?


wrench/reftests/transforms/reftest.list, line 20 at r1 (raw file):

platform(linux) fuzzy(11,4592) == screen-space-blur.yaml screen-space-blur.png
platform(linux,mac) == nested-rotate-x.yaml nested-rotate-x.png
platform(linux,mac) != nested-rotate-x.yaml nested-rotate-x-flat.yaml

would probably make more sense to provide a png here


wrench/src/yaml_frame_reader.rs, line 1558 at r1 (raw file):

        info.rect = bounds;

        let transform_style = yaml["transform-style"]

don't we already read it in some place? (just a minor concern to avoid code duplication)

@emilio
Copy link
Member

emilio left a comment

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @kvark)


webrender/src/spatial_node.rs, line 495 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

if we do have perspective, shouldn't we flatten anyway (for Flat mode), just with perspective multiplied first?

No, perspective is a bit special because of how we represent it in Gecko. My patches for the perspective scrolling stuff will make this clearer.

But basically anything with perspective != [I] will only contain a single transformed ReferenceFrame kid that will do the flattening for us. Servo will need to move to such a model with my patches as well (which is the only right thing to do since the perspective item can scroll independently).


webrender/src/util.rs, line 573 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I don't understand this part. Copy depends on Clone, but you implement Clone as a copy operation?

Yeah, this is just a trick to implement it without duplicating code, it can also be used to copy arrays that are bigger than 32 and other nice things :)

I need the implementation to be separate or to add bounds so that Src: Clone, Dst: Clone to the project function. I think this is better than that.


wrench/reftests/transforms/reftest.list, line 20 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

would probably make more sense to provide a png here

Sure, I'll do both the != and the .png comparison.


wrench/src/yaml_frame_reader.rs, line 1558 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

don't we already read it in some place? (just a minor concern to avoid code duplication)

Yeah, on the caller, but the same way we read transform-origin a bit below. We could pass the values from the caller I guess, but they're quite a few actually, and it probably warrants a different PR.

@kvark
Copy link
Member

kvark left a comment

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @emilio)


webrender/src/spatial_node.rs, line 495 at r1 (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

No, perspective is a bit special because of how we represent it in Gecko. My patches for the perspective scrolling stuff will make this clearer.

But basically anything with perspective != [I] will only contain a single transformed ReferenceFrame kid that will do the flattening for us. Servo will need to move to such a model with my patches as well (which is the only right thing to do since the perspective item can scroll independently).

if that's the assumption at this spot, let's assert on it?


webrender/src/util.rs, line 573 at r1 (raw file):

Previously, emilio (Emilio Cobos Álvarez) wrote…

Yeah, this is just a trick to implement it without duplicating code, it can also be used to copy arrays that are bigger than 32 and other nice things :)

I need the implementation to be separate or to add bounds so that Src: Clone, Dst: Clone to the project function. I think this is better than that.

I don't understand how this makes sense. Could you refer me to some documentation? Copy requires Clone, you shouldn't be able to implement Clone by Copy.

@kvark
Copy link
Member

kvark left a comment

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @emilio)


webrender/src/util.rs, line 573 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I don't understand how this makes sense. Could you refer me to some documentation? Copy requires Clone, you shouldn't be able to implement Clone by Copy.

we played with this a bit: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6d1650f4f5b4817c3ff7e3d2dcc7b4b8

TL:DR: this is a somewhat hacky way to introduce trait chains that don't require any specific implementations
and Copy implementation requires all the fields to be Copy, in addition to the Clone bound, so that we can do it for FastTransform

@emilio
Copy link
Member

emilio left a comment

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kvark)


webrender/src/spatial_node.rs, line 495 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

if that's the assumption at this spot, let's assert on it?

I don't think I can assert on it without making Servo crash. As I said I have more patches that should make this clearer, and I could fix servo then.


webrender/src/util.rs, line 573 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we played with this a bit: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=6d1650f4f5b4817c3ff7e3d2dcc7b4b8

TL:DR: this is a somewhat hacky way to introduce trait chains that don't require any specific implementations
and Copy implementation requires all the fields to be Copy, in addition to the Clone bound, so that we can do it for FastTransform

Sorry, was away on a CSS meeting. I assume this is fine as is? I can just do the manual cumbersome impl, but I'd rather not.

@kvark

kvark approved these changes Dec 12, 2018

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Dec 12, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 12, 2018

📌 Commit e751100 has been approved by kvark

bors-servo added a commit that referenced this pull request Dec 12, 2018

Auto merge of #3401 - emilio:flatten-transform, r=kvark
spatial-node: Flatten transforms properly.

This fixes Gecko bug 1512537.

Fixes #3394

<!-- 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/3401)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 12, 2018

⌛️ Testing commit e751100 with merge b5f1909...

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 12, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing b5f1909 to master...

@bors-servo bors-servo merged commit e751100 into servo:master Dec 12, 2018

2 of 4 checks passed

Taskcluster (pull_request) TaskGroup: failure
Details
code-review/reviewable 2 discussions left (kvark)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@emilio emilio deleted the emilio:flatten-transform branch Dec 12, 2018

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 13, 2018

Bug 1513682 - Update webrender to commit b5f190951f27dd04067489b9fbbe…
…b87f55765f57 (WR PR #3401). r=kats

servo/webrender#3401

Differential Revision: https://phabricator.services.mozilla.com/D14353

--HG--
extra : moz-landing-system : lando

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 13, 2018

Bug 1512537 - Let the reference frame know about the transform style.…
… r=kats

This is the only Gecko side change needed for servo/webrender#3401.

Differential Revision: https://phabricator.services.mozilla.com/D14247

--HG--
extra : moz-landing-system : lando

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 13, 2018

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment