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

Move primitive origin from template data to the instance. #3386

Merged
merged 2 commits into from Dec 6, 2018

Conversation

gw3583
Copy link
Contributor

@gw3583 gw3583 commented Dec 5, 2018

This allows sharing of primitive templates where the only difference
between two primitives is the local space position.

This also means that interning works as expected for primitives
sent by Gecko that contain baked in scroll offsets.

This patch changes the primitive rect only - the next step is a
follow up patch that makes the clip rect, and a few other params
in PrimitiveTemplateKind to also be in a true local space.


This change is Reviewable

@gw3583
Copy link
Contributor Author

gw3583 commented Dec 5, 2018

@gw3583
Copy link
Contributor Author

gw3583 commented Dec 5, 2018

Pushed a follow up commit that normalizes the local clip rect. For some reason I can't push a try run at the moment, but I'll paste one here once I can.

@gw3583
Copy link
Contributor Author

gw3583 commented Dec 5, 2018

@gw3583
Copy link
Contributor Author

gw3583 commented Dec 5, 2018

Hmmm, that second try run is very broken - investigating.

@gw3583
Copy link
Contributor Author

gw3583 commented Dec 5, 2018

Fixed up the last commit, and kicked off a new try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08c72f439b92112bec5d68ce06598d06869836a3

@gw3583
Copy link
Contributor Author

gw3583 commented Dec 5, 2018

That try run looks much better 👍

This allows sharing of primitive templates where the only difference
between two primitives is the local space position.

This also means that interning works as expected for primitives
sent by Gecko that contain baked in scroll offsets.

This patch changes the primitive rect only - the next step is a
follow up patch that makes the clip rect, and a few other params
in PrimitiveTemplateKind to also be in a true local space.

Also normalize the clip rect to the primitive origin.
@gw3583
Copy link
Contributor Author

gw3583 commented Dec 6, 2018

Re-implemented on top of the separate interning PR, since the rebase had too many conflicts.

Kicked off a new pending try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63766afea3cd0d5f1a8f929bd9528d4c52e817fb

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concern

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


webrender/src/display_list_flattener.rs, line 1048 at r1 (raw file):

        DocumentResources: InternerMut<P>,
    {
        let normalized_clip_rect = info.clip_rect

the "normalized" part is a bit confusing to me for having a different meaning in vector math
can we call it relative_clip_rect?
We might need to go as far as creating a separate space for that


webrender/src/display_list_flattener.rs, line 1049 at r1 (raw file):

    {
        let normalized_clip_rect = info.clip_rect
            .translate(&LayoutVector2D::new(-info.rect.origin.x, -info.rect.origin.y))

nit: can we use to_vector()?


webrender/src/picture.rs, line 626 at r1 (raw file):

        let clip_rect = prim_data
            .normalized_clip_rect
            .translate(&LayoutVector2D::new(prim_instance.prim_origin.x, prim_instance.prim_origin.y));

nit: can we use prim_instance.prim_origin.to_vector()?


webrender/src/picture.rs, line 1345 at r1 (raw file):

                let clip_rect = prim_data
                    .normalized_clip_rect
                    .translate(&LayoutVector2D::new(prim_instance.prim_origin.x, prim_instance.prim_origin.y));

same here

@gw3583
Copy link
Contributor Author

gw3583 commented Dec 6, 2018

@kvark Thanks, addressed those comments. And I agree, I think introducing a primitive local space in the future probably makes sense. 👍

@gw3583
Copy link
Contributor Author

gw3583 commented Dec 6, 2018

Try looks good.

@bors-servo r=kvark

@gw3583
Copy link
Contributor Author

gw3583 commented Dec 6, 2018

@bors-servo r=kvark

@jdm
Copy link
Member

jdm commented Dec 6, 2018

@bors-servo ping

@bors-servo
Copy link
Contributor

😪 I'm awake I'm awake

@jdm
Copy link
Member

jdm commented Dec 6, 2018

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

📌 Commit aaeb3d8 has been approved by kvark

bors-servo pushed a commit that referenced this pull request Dec 6, 2018
Move primitive origin from template data to the instance.

This allows sharing of primitive templates where the only difference
between two primitives is the local space position.

This also means that interning works as expected for primitives
sent by Gecko that contain baked in scroll offsets.

This patch changes the primitive rect only - the next step is a
follow up patch that makes the clip rect, and a few other params
in PrimitiveTemplateKind to also be in a true local space.

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

⌛ Testing commit aaeb3d8 with merge 694726e...

@gw3583
Copy link
Contributor Author

gw3583 commented Dec 6, 2018

This looks like another 2-3% win on dl_mutate from the try run.

@bors-servo
Copy link
Contributor

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

@bors-servo bors-servo merged commit aaeb3d8 into servo:master Dec 6, 2018
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 7, 2018
…8e9867e7f8ea (WR PR #3386). r=kats

servo/webrender#3386

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

--HG--
extra : moz-landing-system : lando
mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 7, 2018
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…8e9867e7f8ea (WR PR #3386). r=kats

servo/webrender#3386

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

UltraBlame original commit: 9d91003755a6fae6fb121b1ea115621f4911ae09
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
…8e9867e7f8ea (WR PR #3386). r=kats

servo/webrender#3386

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

UltraBlame original commit: 9d91003755a6fae6fb121b1ea115621f4911ae09
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
…8e9867e7f8ea (WR PR #3386). r=kats

servo/webrender#3386

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

UltraBlame original commit: 9d91003755a6fae6fb121b1ea115621f4911ae09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants