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

Handle scale + offset as same coordinate system. #3046

Merged
merged 2 commits into from Sep 12, 2018

Conversation

Projects
None yet
3 participants
@gw3583
Contributor

gw3583 commented Sep 12, 2018

Apart from simple translations, the most common transform that
is encountered in WR is a scale + offset combination. This is
used especially heavily on animations, and hi-dpi screens.

Previously, WR would consider this a different coordinate
system, which means falling back to clip masks for many
cases (e.g. a rectangle in root coordinate system that has
a partial overlap with a primitive in a scaled coord system).

However, these scale + offset transforms still result in an
axis-aligned rectangle between the two coordinate systems. Thus,
we can actually consider them to be the same coordinate system.

The outcome of this is that we can apply clip rectangles as
local clip rects in the vertex shader. This greatly reduces
the number of clip masks that we allocate in such situations.


This change is Reviewable

Handle scale + offset as same coordinate system.
Apart from simple translations, the most common transform that
is encountered in WR is a scale + offset combination. This is
used especially heavily on animations, and hi-dpi screens.

Previously, WR would consider this a different coordinate
system, which means falling back to clip masks for many
cases (e.g. a rectangle in root coordinate system that has
a partial overlap with a primitive in a scaled coord system).

However, these scale + offset transforms still result in an
axis-aligned rectangle between the two coordinate systems. Thus,
we can actually consider them to be the same coordinate system.

The outcome of this is that we can apply clip rectangles as
local clip rects in the vertex shader. This greatly reduces
the number of clip masks that we allocate in such situations.
@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 12, 2018

Contributor

r? @kvark
cc @jrmuizel

Try run here (pending but looks good far, minus a few unrelated intermittents):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f66aa501d7002c7aaeab99e22871a29a26d7ff79&selectedJob=198777009

Fixes: https://bugzilla.mozilla.org/show_bug.cgi?id=1488629 but anecdotally improves GPU clipping time quite significantly on a lot of pages.

Contributor

gw3583 commented Sep 12, 2018

r? @kvark
cc @jrmuizel

Try run here (pending but looks good far, minus a few unrelated intermittents):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f66aa501d7002c7aaeab99e22871a29a26d7ff79&selectedJob=198777009

Fixes: https://bugzilla.mozilla.org/show_bug.cgi?id=1488629 but anecdotally improves GPU clipping time quite significantly on a lot of pages.

@kvark

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @gw3583)


webrender/src/clip.rs, line 203 at r1 (raw file):

enum ClipSpaceConversion {
    Local,
    ScaleOffset(ScaleOffset),

I like how this enum variant got richer semantics now to differentiate itself further from Local


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

//           way the current clip-scroll tree works.
#[derive(Debug, Clone, Copy)]
pub struct ScaleOffset {

need to document that this is a transformation composed to scaling and offset, where the scaling is applied first


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

        //  - Any positive value present in sx,sy (avoid negative for reflection/rotation)

        if m.m11 < NEARLY_ZERO ||

should we really discard zero scaling for ScaleOffset? I mean, it's not worse than multiplying by a matrix


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

    }

    pub fn accumulate(&self, other: &ScaleOffset) -> Self {

similarly to eucld::Transform, this function name should make it clear if the other is applied before or after self


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

                self.scale.y * other.scale.y,
            ),
            offset: Vector2D::new(

can't we use the vector operations here?


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

    }

    pub fn subtract(&self, other: &ScaleOffset) -> Self {

we need to find a better name. Alternatively, if there is are inverse and apply_before methods, users will be able to do b.inverse().apply_before(a) instead of the current a.subtract(b)


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

    }

    pub fn map<F, T>(&self, rect: &TypedRect<f32, F>) -> TypedRect<f32, T> {

let's rename to map_rect?


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

    }

    pub fn to_inverse_transform<F, T>(&self) -> TypedTransform3D<f32, F, T> {

first, this isn't correct: inv(Offset * Scale) = inv(Scale) * inv(Offset)
So when inversing, the order of operations changes, and thus you'd need -self.offset.x / self.scale.x and so on.
How does it pass the reftests in the current state? maybe I misunderstand the intent of this method?


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

    pub fn to_inverse_transform<F, T>(&self) -> TypedTransform3D<f32, F, T> {
        TypedTransform3D::row_major(

secondly, let's just add inverse() method to ScaleOffset, and the client would be able to chain it with to_transform for the same effect

@gw3583

Reviewable status: 1 of 5 files reviewed, 9 unresolved discussions (waiting on @kvark and @gw3583)


webrender/src/clip.rs, line 203 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I like how this enum variant got richer semantics now to differentiate itself further from Local

Done.


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

Previously, kvark (Dzmitry Malyshau) wrote…

need to document that this is a transformation composed to scaling and offset, where the scaling is applied first

Done.


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

Previously, kvark (Dzmitry Malyshau) wrote…

should we really discard zero scaling for ScaleOffset? I mean, it's not worse than multiplying by a matrix

Changed to only bail out on sx,sy < 0.


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

Previously, kvark (Dzmitry Malyshau) wrote…

similarly to eucld::Transform, this function name should make it clear if the other is applied before or after self

Done.


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

Previously, kvark (Dzmitry Malyshau) wrote…

can't we use the vector operations here?

mul/scale is not defined for vec * vec, unless I'm missing something (probably!).


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

Previously, kvark (Dzmitry Malyshau) wrote…

we need to find a better name. Alternatively, if there is are inverse and apply_before methods, users will be able to do b.inverse().apply_before(a) instead of the current a.subtract(b)

I changed it to 'difference' for now - is that better? I do agree this interface could be better defined, but hopefully we can do that as a follow up.


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

Previously, kvark (Dzmitry Malyshau) wrote…

let's rename to map_rect?

Done.


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

Previously, kvark (Dzmitry Malyshau) wrote…

first, this isn't correct: inv(Offset * Scale) = inv(Scale) * inv(Offset)
So when inversing, the order of operations changes, and thus you'd need -self.offset.x / self.scale.x and so on.
How does it pass the reftests in the current state? maybe I misunderstand the intent of this method?

Oops, good point! I'm guessing the reftests don't exercise that code path :|


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

Previously, kvark (Dzmitry Malyshau) wrote…

secondly, let's just add inverse() method to ScaleOffset, and the client would be able to chain it with to_transform for the same effect

Done.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 12, 2018

Contributor

I'll kick off another try run shortly, to make sure I didn't break anything when addressing those comments.

Contributor

gw3583 commented Sep 12, 2018

I'll kick off another try run shortly, to make sure I didn't break anything when addressing those comments.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583
Contributor

gw3583 commented Sep 12, 2018

@kvark kvark referenced this pull request Sep 12, 2018

Open

Multi-dimensional scale #304

@kvark

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


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

Previously, gw3583 (Glenn Watson) wrote…

Done.

aaaand we got a problem - the comment contradicts the implementation now :)
The implementation adds self.offset, which means the self is applied last.


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

Previously, gw3583 (Glenn Watson) wrote…

mul/scale is not defined for vec * vec, unless I'm missing something (probably!).

You are correct, it's not there yet. Filed servo/euclid#304


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

Previously, gw3583 (Glenn Watson) wrote…

I changed it to 'difference' for now - is that better? I do agree this interface could be better defined, but hopefully we can do that as a follow up.

sorry, I'm still confused. Difference works best for scalar numbers and simple vectors. As soon as you have heterogeneous data (like offset * scale), difference term becomes confusing. I mean, you can say it's a different in multiplicative space, that's not what we do for matrices, so I don't think we should be doing it here either. In general, we should try adhering to matrix/transform naming convention and semantics here, leaving the fact of simpler representation as a internal detail.


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

Previously, gw3583 (Glenn Watson) wrote…

Oops, good point! I'm guessing the reftests don't exercise that code path :|

I'm afraid I can't not ask for a WR reftest then. Would this be possible to represent with one? Perhaps, after the merge even

@gw3583

Reviewable status: 4 of 5 files reviewed, 4 unresolved discussions (waiting on @kvark and @gw3583)


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

Previously, kvark (Dzmitry Malyshau) wrote…

You are correct, it's not there yet. Filed servo/euclid#304

Cool, we can tidy this up once that is available in WR.


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

Previously, kvark (Dzmitry Malyshau) wrote…

sorry, I'm still confused. Difference works best for scalar numbers and simple vectors. As soon as you have heterogeneous data (like offset * scale), difference term becomes confusing. I mean, you can say it's a different in multiplicative space, that's not what we do for matrices, so I don't think we should be doing it here either. In general, we should try adhering to matrix/transform naming convention and semantics here, leaving the fact of simpler representation as a internal detail.

The intent is that if you have a SO from coord system 1 -> 4, and another one from 1 -> 2, this function, when called on those, will return a SO that represents 2 -> 4. Can you suggest what you want this to be called?


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

Previously, kvark (Dzmitry Malyshau) wrote…

I'm afraid I can't not ask for a WR reftest then. Would this be possible to represent with one? Perhaps, after the merge even

Yes, I'll add one as a follow up.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 12, 2018

Contributor

@kvark Addressed comments.

Contributor

gw3583 commented Sep 12, 2018

@kvark Addressed comments.

@kvark

kvark approved these changes Sep 12, 2018

:lgtm:

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kvark)


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

Previously, gw3583 (Glenn Watson) wrote…

The intent is that if you have a SO from coord system 1 -> 4, and another one from 1 -> 2, this function, when called on those, will return a SO that represents 2 -> 4. Can you suggest what you want this to be called?

My preference would be to not call it anything, and just have the client combining inverse() and accumulate as they need, but I don't want this to block the changes.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 12, 2018

Contributor

@kvark Ah, fair enough - I'll make that change as a follow up. Thanks!

Contributor

gw3583 commented Sep 12, 2018

@kvark Ah, fair enough - I'll make that change as a follow up. Thanks!

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark
Member

kvark commented Sep 12, 2018

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 12, 2018

Contributor

📌 Commit 417968e has been approved by kvark

Contributor

bors-servo commented Sep 12, 2018

📌 Commit 417968e has been approved by kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 12, 2018

Contributor

⌛️ Testing commit 417968e with merge 70edb5f...

Contributor

bors-servo commented Sep 12, 2018

⌛️ Testing commit 417968e with merge 70edb5f...

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

Auto merge of #3046 - gw3583:scale-coord-system-2, r=kvark
Handle scale + offset as same coordinate system.

Apart from simple translations, the most common transform that
is encountered in WR is a scale + offset combination. This is
used especially heavily on animations, and hi-dpi screens.

Previously, WR would consider this a different coordinate
system, which means falling back to clip masks for many
cases (e.g. a rectangle in root coordinate system that has
a partial overlap with a primitive in a scaled coord system).

However, these scale + offset transforms still result in an
axis-aligned rectangle between the two coordinate systems. Thus,
we can actually consider them to be the same coordinate system.

The outcome of this is that we can apply clip rectangles as
local clip rects in the vertex shader. This greatly reduces
the number of clip masks that we allocate in such situations.

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

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 12, 2018

Contributor

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

Contributor

bors-servo commented Sep 12, 2018

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

@bors-servo bors-servo merged commit 417968e into servo:master Sep 12, 2018

3 of 4 checks passed

code-review/reviewable 1 discussion left (gw3583)
Details
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