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

Proper near plane splitting #2947

Merged
merged 2 commits into from Aug 3, 2018

Conversation

Projects
None yet
3 participants
@kvark
Member

kvark commented Aug 1, 2018

The PR is built on the shoulders of #2913, #2741, servo/euclid#277, servo/euclid#291, servo/plane-split#12, and (last but not the least!) servo/plane-split#15

It uses the new clipping semantics in plane-split crate uniformly for both bounding box computation and 3d plane splitting itself, ensuring that no incorrect perspective divisions are performed (and closing quite a few TODO entries). Also adds a small reftest for the latter.

Fixes #2272


This change is Reviewable

@kvark kvark requested a review from gw3583 Aug 1, 2018

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 1, 2018

Contributor

Wow, nice! I'll review this today. Could you kick off a try-push in the meantime?

Contributor

gw3583 commented Aug 1, 2018

Wow, nice! I'll review this today. Could you kick off a try-push in the meantime?

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark
Member

kvark commented Aug 2, 2018

Launched a try in https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1fc20bb9928bf6e3ed055fb9bb69c5250c8d784
Also confirmed the bugzilla case working:
screenshot from 2018-08-01 23-03-58

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 2, 2018

Contributor

There are several new PASS results in the try 🎉 but there's also a few failures. Each of them seem to be the same assert:

hread 'WRRenderBackend#4' panicked at 'assertion failed: edge2.square_length() > T::approx_epsilon()', third_party\rust\plane-split\src\polygon.rs:133:9
Contributor

gw3583 commented Aug 2, 2018

There are several new PASS results in the try 🎉 but there's also a few failures. Each of them seem to be the same assert:

hread 'WRRenderBackend#4' panicked at 'assertion failed: edge2.square_length() > T::approx_epsilon()', third_party\rust\plane-split\src\polygon.rs:133:9
@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 2, 2018

Contributor

The change basically looks good to me, but I'll take another pass over it once we get that assert failure fixed :)

Contributor

gw3583 commented Aug 2, 2018

The change basically looks good to me, but I'll take another pass over it once we get that assert failure fixed :)

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark

kvark Aug 3, 2018

Member

Addressed the try push issues in servo/plane-split#18
Launched a try push with those plane-split changes (overridden with a cargo patch):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8225f77003bdf6bb87ecd0494e2dd8b779210d3&selectedJob=191881097

So far, it's a bunch of intermittents plus the expected unexpected passes.
@gw3583 please take your extra pass ;) no code is changed on this side (other than pulling the patched plane-split crate).

Member

kvark commented Aug 3, 2018

Addressed the try push issues in servo/plane-split#18
Launched a try push with those plane-split changes (overridden with a cargo patch):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8225f77003bdf6bb87ecd0494e2dd8b779210d3&selectedJob=191881097

So far, it's a bunch of intermittents plus the expected unexpected passes.
@gw3583 please take your extra pass ;) no code is changed on this side (other than pulling the patched plane-split crate).

@gw3583

Try run looks good to me!

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Aug 3, 2018

Contributor

@bors-servo r+ 🚀

Contributor

gw3583 commented Aug 3, 2018

@bors-servo r+ 🚀

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 3, 2018

Contributor

📌 Commit d331402 has been approved by gw3583

Contributor

bors-servo commented Aug 3, 2018

📌 Commit d331402 has been approved by gw3583

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 3, 2018

Contributor

⌛️ Testing commit d331402 with merge c939a61...

Contributor

bors-servo commented Aug 3, 2018

⌛️ Testing commit d331402 with merge c939a61...

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

Auto merge of #2947 - kvark:near-split, r=gw3583
Proper near plane splitting

The PR is built on the shoulders of #2913, #2741, servo/euclid#277, servo/euclid#291, servo/plane-split#12, and (last but not the least!) servo/plane-split#15

It uses the new clipping semantics in `plane-split` crate uniformly for both bounding box computation and 3d plane splitting itself, ensuring that no incorrect perspective divisions are performed (and closing quite a few TODO entries). Also adds a small reftest for the latter.

Fixes #2272

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

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Aug 3, 2018

Contributor

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: gw3583
Pushing c939a61 to master...

Contributor

bors-servo commented Aug 3, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: gw3583
Pushing c939a61 to master...

@bors-servo bors-servo merged commit d331402 into servo:master Aug 3, 2018

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@kvark kvark deleted the kvark:near-split branch Aug 4, 2018

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