Clip against near plane for bounds calculus #2741

kvark · 2018-05-08T17:14:19Z

Fixes #2389
Fixes #2272
Improves #2429 a great deal (there are general plane splitting issues in there still)

TODO:

reviews
green CI
try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddccbd9f16aea173fe950293f1a4f429650dea6a

This change is

bors-servo · 2018-05-08T22:02:13Z

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

kvark · 2018-05-09T03:11:33Z

Inviting reviewers! cc @glennw

mrobinson · 2018-05-09T07:11:37Z

webrender/src/clip_scroll_node.rs


        // All clipping ClipScrollNodes should have outer rectangles, because they never
        // use the BorderCorner clip type and they always have at last one non-ClipOut
        // Rectangle ClipSource.
-        let screen_outer_rect = screen_outer_rect.expect("Clipping node didn't have outer rect.");
+        let screen_outer_rect = match screen_outer_rect {


Do ClipScrollNodes not always have an outer rectangle now?

In this code, it may be None if either local_outer_rect of the clip source is None, or the computed bounding rect is outside the screen.

For ClipScrollNodes, the local_outer_rect is never None. We use None to indicate situations where an outer rect cannot be calculated and an empty rectangle to indicate a situation where the clip rect is empty. I think the case where the outer bounds do not intersect with the screen rect is an empty clip rectangle situation. In order to avoid having None do double duty (and subtly breaking other parts of the code), I think we should maintain the expect here and have the non-intersection case produce an empty rectangle.

mrobinson · 2018-05-09T07:11:37Z

webrender/src/util.rs

-        transform.transform_point2d(&rect.bottom_left()),
-        transform.transform_point2d(&rect.bottom_right()),
+    screen_bounds: Option<&DeviceIntRect>,
+) -> Option<DeviceIntRect> {


I recall this function as being extremely performance sensitive, so is there any way to have a fast path when it isn't necessary to do all of these calculations?

The way it's build is to avoid extra steps unless necessary. So the old transform_point2d for each corner is equivalent to transform_point_2d_homogeneous followed by to_point2d at the end.
Extra overhead may come from the following sources:

more stuff being moved around (copies are the doom of Rust code)

comparing 4 f32 values to 0 in homogens.iter().any(|h| h.w <= 0.0)

worse instruction cache utilization for the function body being grown

Overall, I think it's a reasonable compromise, and it's not clear to me if we can do better here.

One thing we could do here is to detect when transformation doesn't project into three dimensions and to use the old code for this situation. You could have this path be an early return and avoid the long if statement in the body.

kvark · 2018-05-09T11:32:56Z

I'm a bit puzzled over AppVeyor reftest failure...

kvark

The try push also looks good.

kvark · 2018-05-09T13:03:13Z

webrender/src/clip_scroll_node.rs


        // All clipping ClipScrollNodes should have outer rectangles, because they never
        // use the BorderCorner clip type and they always have at last one non-ClipOut
        // Rectangle ClipSource.
-        let screen_outer_rect = screen_outer_rect.expect("Clipping node didn't have outer rect.");
+        let screen_outer_rect = match screen_outer_rect {


In this code, it may be None if either local_outer_rect of the clip source is None, or the computed bounding rect is outside the screen.

kvark · 2018-05-09T13:03:13Z

webrender/src/util.rs

-        transform.transform_point2d(&rect.bottom_left()),
-        transform.transform_point2d(&rect.bottom_right()),
+    screen_bounds: Option<&DeviceIntRect>,
+) -> Option<DeviceIntRect> {


The way it's build is to avoid extra steps unless necessary. So the old transform_point2d for each corner is equivalent to transform_point_2d_homogeneous followed by to_point2d at the end.
Extra overhead may come from the following sources:

more stuff being moved around (copies are the doom of Rust code)

comparing 4 f32 values to 0 in homogens.iter().any(|h| h.w <= 0.0)

worse instruction cache utilization for the function body being grown

Overall, I think it's a reasonable compromise, and it's not clear to me if we can do better here.

kvark · 2018-05-09T13:06:52Z

@mrobinson thanks for taking a look! I think we are all set, unless there are more concerns.

mrobinson · 2018-05-09T15:26:22Z

wrench/src/main.rs

@@ -15,7 +15,7 @@ extern crate core_graphics;
 extern crate crossbeam;
 #[cfg(target_os = "windows")]
 extern crate dwrote;
-#[cfg(feature = "logging")]
+#[cfg(feature = "env_logger")]


I think this was landed in another patch?

mrobinson

Sorry for the weird review on my part! This is looking pretty reasonable to me, but I have a few minor nits that I think could be addressed before landing.


        Augmented screen bounds calculation involving plane clipper for cases…

… where transformed polygons intersect the near plane.

kvark · 2018-05-09T15:34:18Z

@mrobinson
Rebased and fixed the local outer rect now (hopefully).

One thing we could do here is to detect when transformation doesn't project into three dimensions and to use the old code for this situation. You could have this path be an early return and avoid the long if statement in the body.

It would involve more logic duplication and control flow tweaks, so I'm not sure it's worth it. Maybe we can leave a TODO comment in the code saying to keep an eye for this branch when profiling?

kvark · 2018-05-09T18:39:14Z

Updated try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dd1dfe5c8453fe34e37bcfbfd23f54e4bcc9a2e

kvark · 2018-05-09T20:39:02Z

I don't want this to stall any longer.
@bors-servo r=mrobinson

bors-servo · 2018-05-09T20:39:03Z

📌 Commit cafeb7a has been approved by mrobinson

bors-servo · 2018-05-09T21:38:53Z

⌛ Testing commit cafeb7a with merge fac0040...


        Auto merge of #2741 - kvark:perspective, r=mrobinson

Clip against near plane for bounds calculus Fixes #2389 Fixes #2272 Improves #2429 a great deal (there are general plane splitting issues in there still) TODO: - [x] reviews - [x] green CI - [x] try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddccbd9f16aea173fe950293f1a4f429650dea6a  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2741)

bors-servo · 2018-05-09T22:36:58Z

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


        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  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2947)

kvark force-pushed the kvark:perspective branch from 2cf4c77 to fbaf67b May 8, 2018

kvark force-pushed the kvark:perspective branch 3 times, most recently from 176e93c to cf5c87b May 9, 2018

kvark changed the title ~~[WIP] Clip against near plane for bounds calculus~~ Clip against near plane for bounds calculus May 9, 2018

mrobinson reviewed May 9, 2018

View changes

kvark reviewed May 9, 2018

View changes

kvark force-pushed the kvark:perspective branch from cf5c87b to 9ed15e4 May 9, 2018

mrobinson reviewed May 9, 2018

View changes

Augmented screen bounds calculation involving plane clipper for cases…

Loading status checks…

cafeb7a

… where transformed polygons intersect the near plane.

kvark force-pushed the kvark:perspective branch from 9ed15e4 to cafeb7a May 9, 2018

bors-servo merged commit cafeb7a into servo:master May 9, 2018
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

kvark deleted the kvark:perspective branch May 10, 2018

kvark mentioned this pull request May 16, 2018

regression: Animation rendered all over the place with webrender #2272

Closed

kvark mentioned this pull request Aug 1, 2018

Proper near plane splitting #2947

Merged

servo / webrender

Clip against near plane for bounds calculus #2741

Clip against near plane for bounds calculus #2741

kvark commented May 8, 2018 •

edited

bors-servo commented May 8, 2018

kvark commented May 9, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kvark commented May 9, 2018

kvark left a comment

This comment has been minimized.

This comment has been minimized.

kvark commented May 9, 2018

This comment has been minimized.

mrobinson left a comment

kvark commented May 9, 2018

kvark commented May 9, 2018

kvark commented May 9, 2018

bors-servo commented May 9, 2018

bors-servo commented May 9, 2018

bors-servo commented May 9, 2018

servo / webrender

Join GitHub today

Clip against near plane for bounds calculus #2741

Clip against near plane for bounds calculus #2741

Conversation

kvark commented May 8, 2018 • edited

bors-servo commented May 8, 2018

kvark commented May 9, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mrobinson May 9, 2018 • edited

kvark commented May 9, 2018

kvark left a comment

This comment has been minimized.

This comment has been minimized.

kvark commented May 9, 2018

This comment has been minimized.

mrobinson left a comment

kvark commented May 9, 2018

kvark commented May 9, 2018

kvark commented May 9, 2018

bors-servo commented May 9, 2018

bors-servo commented May 9, 2018

bors-servo commented May 9, 2018

kvark commented May 8, 2018 •

edited

mrobinson May 9, 2018 •

edited