Skip to content
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

Better inversible transformations #2913

Merged
merged 2 commits into from Jul 20, 2018
Merged

Better inversible transformations #2913

merged 2 commits into from Jul 20, 2018

Conversation

@kvark
Copy link
Member

kvark commented Jul 19, 2018

Hopefully addresses #2911 (how can I verify?)
r? @nical


This change is Reviewable

@nical
nical approved these changes Jul 19, 2018
Copy link
Collaborator

nical left a comment

Looks good to me (with the extra commits removed). To test it you can add this patch on top of this revision https://treeherder.mozilla.org/#/jobs?repo=try&revision=04c6bc71074245fa9d8438a78922e71fbd36dff7&selectedJob=188748580 and run the crashtests locally ./mach crashtest.

@kvark kvark force-pushed the kvark:transform branch from 86598fe to 2355996 Jul 19, 2018
@nical
Copy link
Collaborator

nical commented Jul 19, 2018

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jul 19, 2018

📌 Commit 2355996 has been approved by nical

@staktrace
Copy link
Contributor

staktrace commented Jul 19, 2018

bors-servo added a commit that referenced this pull request Jul 19, 2018
Better inversible transformations

Fixes #2911 (how can I verify?)
r? @nical

<!-- 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/2913)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 19, 2018

Testing commit 2355996 with merge 412a0b5...

@staktrace
Copy link
Contributor

staktrace commented Jul 19, 2018

This patch appears to be insufficient, we're still getting panics (see try push above). It's hitting the all the points are supposed to be transformable after near plane clipping panic path.

@kvark
Copy link
Member Author

kvark commented Jul 19, 2018

@staktrace this makes sense. I'm still working (closest WR bug is #2272) on that part of code (just switching on and off, mostly off, occasionally), and I know it's not yet entirely correct, so unwrapping does no good there (until it's fixed). How about a temporary workaround in 14ebffb ?

@staktrace
Copy link
Contributor

staktrace commented Jul 19, 2018

@staktrace
Copy link
Contributor

staktrace commented Jul 19, 2018

Above try push still had failures, this time from this line per my local repro on layout/reftests/transform-3d/1035611-1.html

@kvark kvark force-pushed the kvark:transform branch from 14ebffb to 5d1eddc Jul 19, 2018
@kvark
Copy link
Member Author

kvark commented Jul 19, 2018

@staktrace thanks for tracing this crash down! I updated the second commit here to somewhat work around it.

Note: the workaround is super unsound and in normal circumstances shouldn't be merged. However, the old code was essentially producing undefined values (behind the near plane) by that transformation, so defaulting to concrete ones, while still wrong, is a step forward.

@staktrace
Copy link
Contributor

staktrace commented Jul 19, 2018

I did a local build with that patch (try still going here) and it crashes still. Same test as last time, but a different spot:

#10 0x00007fffe95c50bc in _$LT$core..option..Option$LT$T$GT$$GT$::unwrap::h85c542007de8ef77 (self=...) at /checkout/src/libcore/macros.rs:20
#11 0x00007fffe95cab79 in webrender::batch::make_polygon::h377795bdcde61f94 (rect=..., transform=<optimized out>, anchor=<optimized out>) at gfx/webrender/src/batch.rs:1717
#12 0x00007fffe95c8c34 in webrender::batch::AlphaBatchBuilder::add_prim_to_batch::h74e57828c73cc409 (self=0x7fffc15fa300, transform_id=..., prim_index=..., ctx=<optimized out>, gpu_cache=0x7fffc15fc9b0, render_tasks=0x7fffc15fa660, task_id=..., task_address=..., deferred_resolves=0x7fffc15fa648, splitter=0x7fffc15f8170, content_origin=..., prim_headers=0x7fffc15fa748)
    at gfx/webrender/src/batch.rs:676
#13 0x00007fffe95c88cb in webrender::batch::AlphaBatchBuilder::add_run_to_batch::hfc4831300e0e33d3 (self=0x7fffc15fa300, run=<optimized out>, transform_id=..., ctx=0x7fffc15fa5d0, gpu_cache=<optimized out>, render_tasks=<optimized out>, task_id=..., task_address=..., deferred_resolves=<optimized out>, splitter=<optimized out>, content_origin=..., prim_headers=<optimized out>)
    at gfx/webrender/src/batch.rs:559

Polygon::from_transformed_rect(rect.cast(), mat, anchor).unwrap()

@kvark kvark force-pushed the kvark:transform branch from 5d1eddc to a542756 Jul 19, 2018
@jrmuizel
Copy link
Contributor

jrmuizel commented Jul 20, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2018

📌 Commit a542756 has been approved by jrmuizel

@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2018

Testing commit a542756 with merge 9f21ee5...

bors-servo added a commit that referenced this pull request Jul 20, 2018
Better inversible transformations

Hopefully addresses #2911 (how can I verify?)
r? @nical

<!-- 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/2913)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 20, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: jrmuizel
Pushing 9f21ee5 to master...

@bors-servo bors-servo merged commit a542756 into servo:master Jul 20, 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 kvark deleted the kvark:transform branch Jul 20, 2018
@kvark
Copy link
Member Author

kvark commented Jul 20, 2018

@jrmuizel the errors in this try push appear from #2891:

thread '' panicked at 'Unable to open shader file: "/builds/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/webrender-fec329b250b1de8f/out/brush_solid.vert": Os { code: 2, kind: NotFound, message: "No such file or directory" }', libcore/result.rs:945:5
11:13:21 INFO - PID 18099 | stack backtrace:

bors-servo added a commit that referenced this pull request Aug 3, 2018
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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.