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

Make batch caching based on z id rather than prim index. #3221

Merged
merged 1 commit into from Oct 23, 2018

Conversation

Projects
None yet
4 participants
@gw3583
Collaborator

gw3583 commented Oct 20, 2018

This is a necessary change for picture caching, because in the
future pictures won't have a primitive index. It is also a
better way of expressing the intent of the batch caching anyway
(that the overlap detection should be invoked next time we have
a primitive with a different z value).


This change is Reviewable

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 20, 2018

Let's not merge this until #3131 lands - to avoid more rebase issues in that PR.

@kvark

kvark approved these changes Oct 22, 2018

It's a bit unclear to me what your next steps are, and why primitive index needs to go in the first place. But overall the change looks fine :)

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


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

        let mut prim_headers = PrimitiveHeaders::new();
        // Used to generated a unique z-buffer value per primitive.
        let mut z_generator = ZBufferIdGenerator::new();

any reason we don't put it into RenderTargetContext?

@gw3583

The idea is that (eventually) there won't be a primitive index, there will just be primitive instances that reference interned primitive templates. This gets us one step closer to that :)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gw3583)


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

Previously, kvark (Dzmitry Malyshau) wrote…

any reason we don't put it into RenderTargetContext?

It's not mutable. We could make the z generator use a Cell in the future perhaps.

@kvark

kvark approved these changes Oct 22, 2018

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 22, 2018

☔️ The latest upstream changes (presumably #3131) made this pull request unmergeable. Please resolve the merge conflicts.

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 22, 2018

Rebased on top of the backface visibility changes.

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 22, 2018

📌 Commit a9af605 has been approved by kvark

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 23, 2018

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 23, 2018

📌 Commit 2d4cf88 has been approved by kvark

@emilio

This comment has been minimized.

Member

emilio commented Oct 23, 2018

(trying to wake up bors, again)

@emilio emilio closed this Oct 23, 2018

@emilio emilio reopened this Oct 23, 2018

@emilio

This comment has been minimized.

Member

emilio commented Oct 23, 2018

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 23, 2018

💡 This pull request was already approved, no need to approve it again.

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 23, 2018

📌 Commit 2d4cf88 has been approved by kvark

Make batch caching based on z id rather than prim index.
This is a necessary change for picture caching, because in the
future pictures won't have a primitive index. It is also a
better way of expressing the intent of the batch caching anyway
(that the overlap detection should be invoked next time we have
a primitive with a different z value).
@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 23, 2018

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 23, 2018

📌 Commit a748e38 has been approved by kvark

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 23, 2018

Manually merging since bors is asleep. I manually rebased this on the last manual merge and ran the reftests locally.

@gw3583 gw3583 merged commit a8817b9 into servo:master Oct 23, 2018

0 of 3 checks passed

Taskcluster (pull_request) TaskGroup: Pending (for pull_request.synchronize)
Details
code-review/reviewable 3 files left (kvark)
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details

@gw3583 gw3583 deleted the gw3583:z-id branch Oct 23, 2018

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