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

Projects
None yet
4 participants
@gw3583
Copy link
Collaborator

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

This comment has been minimized.

@gw3583

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 5, 2018

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

@gw3583 gw3583 force-pushed the gw3583:unit-rect branch from 87f6acb to 449ea63 Dec 5, 2018

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 5, 2018

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 5, 2018

That try run looks much better 👍

@gw3583 gw3583 force-pushed the gw3583:unit-rect branch from 449ea63 to 85f0fcd 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.

Also normalize the clip rect to the primitive origin.

@gw3583 gw3583 force-pushed the gw3583:unit-rect branch from 85f0fcd to b1f55db Dec 6, 2018

@gw3583

This comment has been minimized.

Copy link
Collaborator

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

@kvark
Copy link
Member

kvark left a comment

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

This comment has been minimized.

Copy link
Collaborator

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

@kvark

kvark approved these changes Dec 6, 2018

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 6, 2018

Try looks good.

@bors-servo r=kvark

@gw3583 gw3583 force-pushed the gw3583:unit-rect branch from a8a50a3 to aaeb3d8 Dec 6, 2018

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 6, 2018

@bors-servo r=kvark

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 6, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 6, 2018

😪 I'm awake I'm awake

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Dec 6, 2018

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 6, 2018

📌 Commit aaeb3d8 has been approved by kvark

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

Auto merge of #3386 - gw3583:unit-rect, r=kvark
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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 6, 2018

⌛️ Testing commit aaeb3d8 with merge 694726e...

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 6, 2018

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

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 6, 2018

☀️ 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

3 of 4 checks passed

code-review/reviewable 5 files, 4 discussions left (gw3583, kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

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

Bug 1512482 - Update webrender to commit 694726ea7eee67ea3d8c6586aba2…
…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

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