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

Force all calls to project_rect to include a bounding rect. #2999

Merged
merged 1 commit into from Aug 31, 2018

Conversation

Projects
None yet
3 participants
@gw3583
Contributor

gw3583 commented Aug 31, 2018

This greatly improves the accuracy of the clipper code when
required to calculate the world rect for a primitive that
crosses the near plane.

This patch by itself doesn't fix anything specific, but when
combined with the fixes that are in progress in the
plane-split crate, it will allow correct world rect calculations
for primitives that cross the near plane in various edge cases.


This change is Reviewable

Force all calls to project_rect to include a bounding rect.
This greatly improves the accuracy of the clipper code when
required to calculate the world rect for a primitive that
crosses the near plane.

This patch by itself doesn't fix anything specific, but when
combined with the fixes that are in progress in the
plane-split crate, it will allow correct world rect calculations
for primitives that cross the near plane in various edge cases.
@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 31, 2018

Contributor

r? @kvark

Mea culpa

Pending try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eaec0a34c022601684bbe70b11f6d76d6d8a7c2

I expect the try run to have some orange items due to the previous PRs that have recently landed, so I'll comment on the results once the try completes.

Contributor

gw3583 commented Aug 31, 2018

r? @kvark

Mea culpa

Pending try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eaec0a34c022601684bbe70b11f6d76d6d8a7c2

I expect the try run to have some orange items due to the previous PRs that have recently landed, so I'll comment on the results once the try completes.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 31, 2018

Contributor

Try looks good:

It matches the current baseline https://treeherder.mozilla.org/#/jobs?repo=try&revision=d25ddc2775a1146d2dad8b63fa8d011f49a6222e&selectedJob=196793766 against WR master, except for one new PASS in R1 [1157984-1.html].

Contributor

gw3583 commented Aug 31, 2018

Try looks good:

It matches the current baseline https://treeherder.mozilla.org/#/jobs?repo=try&revision=d25ddc2775a1146d2dad8b63fa8d011f49a6222e&selectedJob=196793766 against WR master, except for one new PASS in R1 [1157984-1.html].

@kvark

kvark approved these changes Aug 31, 2018

let map_local_to_pic = SpaceMapper::new(
ref_spatial_node_index,
pic_bounds,

This comment has been minimized.

@kvark

kvark Aug 31, 2018

Member

looks like the SpaceMapper abstraction is paying off nicely here 👍

@kvark

kvark Aug 31, 2018

Member

looks like the SpaceMapper abstraction is paying off nicely here 👍

@@ -161,6 +166,21 @@ impl<F, T> SpaceMapper<F, T> where F: fmt::Debug {
}
}
pub fn unmap(&self, rect: &TypedRect<f32, T>) -> Option<TypedRect<f32, F>> {

This comment has been minimized.

@kvark

kvark Aug 31, 2018

Member

I think this should be renamed. It sounds like the mapping was reversible in the first place and we just undo it, but in reality inverse_rect_footprint is clearly irreversible.

@kvark

kvark Aug 31, 2018

Member

I think this should be renamed. It sounds like the mapping was reversible in the first place and we just undo it, but in reality inverse_rect_footprint is clearly irreversible.

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark

kvark Aug 31, 2018

Member

This resolves my concern from #2995 review. Thank you!
@bors-servo r+

Member

kvark commented Aug 31, 2018

This resolves my concern from #2995 review. Thank you!
@bors-servo r+

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 31, 2018

Contributor

📌 Commit 9953081 has been approved by kvark

Contributor

bors-servo commented Aug 31, 2018

📌 Commit 9953081 has been approved by kvark

bors-servo added a commit that referenced this pull request Aug 31, 2018

Auto merge of #2999 - gw3583:prim-bounds, r=kvark
Force all calls to project_rect to include a bounding rect.

This greatly improves the accuracy of the clipper code when
required to calculate the world rect for a primitive that
crosses the near plane.

This patch by itself doesn't fix anything specific, but when
combined with the fixes that are in progress in the
plane-split crate, it will allow correct world rect calculations
for primitives that cross the near plane in various edge cases.

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

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 31, 2018

Contributor

⌛️ Testing commit 9953081 with merge 69dcfc7...

Contributor

bors-servo commented Aug 31, 2018

⌛️ Testing commit 9953081 with merge 69dcfc7...

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 31, 2018

Contributor

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

Contributor

bors-servo commented Aug 31, 2018

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

@bors-servo bors-servo merged commit 9953081 into servo:master Aug 31, 2018

3 checks passed

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