Skip to content

/ webrender

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

Take the clip chain's clip rect into account for local_rect computation #2714

Merged
merged 1 commit into from May 3, 2018
Merged

Take the clip chain's clip rect into account for local_rect computation #2714

merged 1 commit into from May 3, 2018

Conversation

@kvark
Copy link
Member

kvark commented May 1, 2018 •
edited by larsbergstrom

Fixes #2567
The clip chain knows that each gradient primitive in each opacity-filtered stacking context is clipped to a 100x100 rectangle, and now it's taken into account, so we no longer allocate/fill/sample 35 2Kx2K targets :)
Try push is pending...

This change is Reviewable
@kvark
Copy link
Member Author

kvark commented May 1, 2018

uh, weird test results:
REFTEST TEST-UNEXPECTED-FAIL | reftests/text/shadow-clipped-text.yaml != reftests/text/blank.yaml | image comparison
REFTEST TEST-END | reftests/text/shadow-clipped-text.yaml != reftests/text/blank.yaml
@kvark
Copy link
Member Author

kvark commented May 1, 2018

Gecko try push looks green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eedff21aa8befdd8ccc4ef350274fbac79296f69&selectedJob=176460474
@kvark
Copy link
Member Author

kvark commented May 1, 2018

The failure appears to be #2374 coming back: a text primitive is culled off while the shadow should stay visible.
@glennw
Copy link
Member

glennw commented May 1, 2018

Can we apply the inflation factor after intersecting with the chain local rect?
@kvark kvark force-pushed the kvark:chain-rect branch 2 times, most recently from 2da3e0f to c878c0e May 2, 2018
@kvark
Take the clip chain's clip rect into account for local_rect computation
Loading status checks…
f72fe5c
@kvark kvark force-pushed the kvark:chain-rect branch from c878c0e to f72fe5c May 2, 2018
@kvark
Copy link
Member Author

kvark commented May 2, 2018

@glennw I don't think this would be correct, since the very testcase failing is because a primitive is culled out while its shadow needs to be visible. Given that the shadow visibility is provided by that inflation radius, if we clip the primitive before inflation, it becomes completely culled.

I believe I found the correct way to do the clipping now, PR is updated.
r? @glennw or @mrobinson
@glennw
Copy link
Member

glennw commented May 2, 2018

Do we need another try run for this change?
@kvark
Copy link
Member Author

kvark commented May 3, 2018

Another try push - https://treeherder.mozilla.org/#/jobs?repo=try&revision=3915f3ab8b85778f7317de38f8c3d0e429ca1021
@glennw
Copy link
Member

glennw commented May 3, 2018

I think this looks right - r=me if try is green, but it might be worth getting @mrobinson to take a look over it too :)
@glennw glennw self-requested a review May 3, 2018
@glennw
glennw approved these changes May 3, 2018
View changes
@mrobinson
mrobinson reviewed May 3, 2018
View changes
Copy link
Member

mrobinson left a comment

This change seems fine to me.
@kvark
Copy link
Member Author

kvark commented May 3, 2018

@bors-servo r=glennw
@bors-servo
Copy link
Contributor

bors-servo commented May 3, 2018

📌 Commit f72fe5c has been approved by glennw
@bors-servo
Copy link
Contributor

bors-servo commented May 3, 2018

⌛ Testing commit f72fe5c with merge 86be31d...
bors-servo added a commit that referenced this pull request May 3, 2018
@bors-servo
Auto merge of #2714 - kvark:chain-rect, r=glennw
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
86be31d 
Take the clip chain's clip rect into account for local_rect computation

Fixes  #2567
The clip chain knows that each gradient primitive in each opacity-filtered stacking context is clipped to a 100x100 rectangle, and now it's taken into account, so we no longer allocate/fill/sample 35 2Kx2K targets :)
Try push is pending...

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

bors-servo commented May 3, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: glennw
Pushing 86be31d to master...
@bors-servo bors-servo merged commit f72fe5c into servo:master May 3, 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:chain-rect branch May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrobinson mrobinson

@glennw glennw

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@kvark @glennw @bors-servo @mrobinson
You can’t perform that action at this time.