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

Reduce the overdraw of border dashes. #3483

Merged
merged 2 commits into from Jan 9, 2019
Merged

Conversation

@nical
Copy link
Collaborator

nical commented Jan 8, 2019

Shrink the dash segments using a conservative rect computed in the vertex shader.
The dash segment rects could be made tighter (using a very simple and safe conservative approximation here).


This change is Reviewable

Shrink the dash segments using a conservative rect computed in the vertex shader.
The dash segment rects could be made tighter (we're using a very simple and safe conservative approximation here).
@nical
Copy link
Collaborator Author

nical commented Jan 8, 2019

The picture below shows the geometry in renderedoc, as you can see the rect is very conservative and could be made tighter.

screenshot from 2019-01-08 14-15-28

I think that it overshoots by a max of 30% (2.0 - sqrt(2)/2) a (still) conservative rect that would assume no curvature on the dashes. We could tweak this but I suspect that this PR is enough to fix #3399 and bug 1510076. Before this PR, each segment covers the entire corner so the wins are already significant.

@nical
Copy link
Collaborator Author

nical commented Jan 8, 2019

This changed a few pixels in the aa of reftests/border/border-suite-2 which are still covered by the segment rect so the small difference must come from vertex interpolation and float precision playing out slightly differently.

@nical
Copy link
Collaborator Author

nical commented Jan 8, 2019

With this we can re-enable the 4 disabled crashtests of bug 1510076 and they pass: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c622c1d46307b5b1f46d1f7d34ecc8890e619ca&selectedJob=220591569

r? @kvark

@kvark
kvark approved these changes Jan 8, 2019
// a strong curvature and we will overshoot by inflating the geometry by
// this amount on each side (sqrt(2) * length(dash) would be enough and we
// compute 2 * approx_length(dash)).
float dash_length = length(aClipParams1.xy - aClipParams2.xy);

This comment has been minimized.

@kvark

kvark Jan 8, 2019

Member

is it possible to process width and height separately here (and get a tighter bound therefore)?

This comment has been minimized.

@nical

nical Jan 9, 2019

Author Collaborator

I may be missing something but since we don't know the orientation of the dash, it's hard to consider the dash width and length separately. So we have to assume the longest side in any direction and potentially the worst orientation (45 degrees).

We could compute the orientation from aClipParams1.xy and aClipParams2.xy which are at the start and end of the dash but that would add a lot of math. To get a really tight rect we'd need to consider the curvature of the dash in the computation which would make that a ton more complicated for a rather small amount of overdraw.

That's why I settled for a very conservative rect. Without extra information we could technically reduce the rect because, again, assuming dashes are rectangles with no curvature (which is not quite true), a sufficiently conservative bounding square would have each side equal to sqrt(2) * max(width, length) (sqrt(2) coming from the worst-case angle of 45 degrees) and this PR uses a factor of 2 instead of sqrt(2). The factor of two is implicit (it's just that we inflate the rect of that quantity on each side).

Because this formula doesn't account for the curvature we'd need some extra headroom to make sure we don't clip dashes with strong curvature but quantifying how much we need is not worth the complexity in my opinion. It's most likely less than the ~30% from this PR though. So if we are willing to accept a magic constant factor that has no mathematical ground, we should be safe multiplying by, say, 0.8 and leave only about 10% on each side for the curvature. I almost put that on the PR but I suspected an arbitrary factor would generate some debate.

I'm not sure real-world pages will benefit much from tighter bounding rects anyway, so I think we should stick to something simple and maybe come back and implement something more complicated if/when we have evidence that it can have an impact.

This comment has been minimized.

@kvark

kvark Jan 9, 2019

Member

I see. Can we have that magic factor to be explicit, at least?

This comment has been minimized.

@nical

nical Jan 9, 2019

Author Collaborator

So right now there isn't really a magic factor (other than that since we are creating a square with edges at ±max(width, length) from the center we get 2*max(width, length) sides). But I only say "2x" because I formulate the problem in my head relative to the length of the edges of the square, but one could think of it as the distance between the egdes and the center of the dash (1x then).

Do you mean adding a factor like the 0.8 I previously mentioned or do you mean I should reword the existing comment?

@kvark
Copy link
Member

kvark commented Jan 9, 2019

Let's refine in follow-ups then :)
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Jan 9, 2019

📌 Commit 5ff8947 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Jan 9, 2019

Testing commit 5ff8947 with merge c8648da...

bors-servo added a commit that referenced this pull request Jan 9, 2019
Reduce the overdraw of border dashes.

Shrink the dash segments using a conservative rect computed in the vertex shader.
The dash segment rects could be made tighter (using a very simple and safe conservative approximation here).

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

bors-servo commented Jan 9, 2019

💔 Test failed - status-appveyor

@kvark
Copy link
Member

kvark commented Jan 9, 2019

@bors-servo retry

bors-servo added a commit that referenced this pull request Jan 9, 2019
Reduce the overdraw of border dashes.

Shrink the dash segments using a conservative rect computed in the vertex shader.
The dash segment rects could be made tighter (using a very simple and safe conservative approximation here).

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

bors-servo commented Jan 9, 2019

Testing commit 5ff8947 with merge a25552a...

@bors-servo
Copy link
Contributor

bors-servo commented Jan 9, 2019

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

@bors-servo bors-servo merged commit 5ff8947 into servo:master Jan 9, 2019
3 checks passed
3 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 10, 2019
…f592ab9fde93 (WR PR #3483). r=kats

servo/webrender#3483

Differential Revision: https://phabricator.services.mozilla.com/D16082

--HG--
extra : moz-landing-system : lando
mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Jan 10, 2019
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…f592ab9fde93 (WR PR #3483). r=kats

servo/webrender#3483

Differential Revision: https://phabricator.services.mozilla.com/D16082

UltraBlame original commit: ce7f9998683947e56d308894aa8e17ee7ce57674
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
…f592ab9fde93 (WR PR #3483). r=kats

servo/webrender#3483

Differential Revision: https://phabricator.services.mozilla.com/D16082

UltraBlame original commit: ce7f9998683947e56d308894aa8e17ee7ce57674
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
…f592ab9fde93 (WR PR #3483). r=kats

servo/webrender#3483

Differential Revision: https://phabricator.services.mozilla.com/D16082

UltraBlame original commit: ce7f9998683947e56d308894aa8e17ee7ce57674
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

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