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

Fix opacity of Clear brushes #3252

Merged
merged 1 commit into from Oct 31, 2018
Merged

Fix opacity of Clear brushes #3252

merged 1 commit into from Oct 31, 2018

Conversation

@kvark
Copy link
Member

kvark commented Oct 30, 2018

@staktrace
Copy link
Contributor

staktrace commented Oct 30, 2018

r+ if try is green

@mstange
Copy link
Contributor

mstange commented Oct 30, 2018

Does this make Clear brushes go into the transparent pass? If so, that would be unfortunate: whatever is behind a Clear brush will never be visible, so ideally we should avoid drawing it.

Clear is opaque in the sense that "whatever is under me will never reach the screen". It is not opaque in the sense "after I draw, alpha will be 1".

@kvark
Copy link
Member Author

kvark commented Oct 31, 2018

Forgot to thank @mstange for hinting about the clear brushes, and now they are saying we need a different fix :) oh well. I believe the translucent() is what the old behavior was like, and it would be nice to fix the regression first before trying to optimize it. And yes, I agree that ideally we'd want to the clear brushes to be done in the opaque pass, just with the color masked out (and only alpha written). Let's take care of this as a follow-up.

@bors-servo r=staktrace

@bors-servo
Copy link
Contributor

bors-servo commented Oct 31, 2018

📌 Commit 8816d43 has been approved by staktrace

bors-servo added a commit that referenced this pull request Oct 31, 2018
Fix opacity of Clear brushes

Follow-up to #3233
Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1502585
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2319bbf4ce04d1af7a785567c09d43884b8c3ca3
cc @staktrace
r? anyone (once the try is green)

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

bors-servo commented Oct 31, 2018

Testing commit 8816d43 with merge 0c69aa5...

@kvark
Copy link
Member Author

kvark commented Oct 31, 2018

Filed #3253 for the follow-up.

@kvark kvark force-pushed the kvark:clear-opacity branch 2 times, most recently from 0aee2a0 to 8816d43 Oct 31, 2018
@staktrace
Copy link
Contributor

staktrace commented Oct 31, 2018

I don't know what bors is up to, but it looks like it decided to not wait for the test merge here, and started testing 3244 instead?

@kvark
Copy link
Member Author

kvark commented Oct 31, 2018

@staktrace I accidentally force-pushed a small update to this branch which was meant for another one, and then force-pushed the old commit back in place. Surprised that bors didn't complain at all, but it did shift the focus on another task that was in the queue, so that's normal.

@bors-servo retry

@gw3583
Copy link
Collaborator

gw3583 commented Oct 31, 2018

Once a force push occurs, I think bors might need a new r+.

@bors-servo r=staktrace

@bors-servo
Copy link
Contributor

bors-servo commented Oct 31, 2018

📌 Commit 8816d43 has been approved by staktrace

@kvark
Copy link
Member Author

kvark commented Oct 31, 2018

@gw3583 the tricky thing is when a force push is done for a commit that was already approved. Bors shouldn't require another r+ there

@bors-servo
Copy link
Contributor

bors-servo commented Oct 31, 2018

Testing commit 8816d43 with merge e162264...

bors-servo added a commit that referenced this pull request Oct 31, 2018
Fix opacity of Clear brushes

Follow-up to #3233
Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1502585
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2319bbf4ce04d1af7a785567c09d43884b8c3ca3
cc @staktrace
r? anyone (once the try is green)

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

bors-servo commented Oct 31, 2018

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

@bors-servo bors-servo merged commit 8816d43 into servo:master Oct 31, 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:clear-opacity branch Oct 31, 2018
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.