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

cs_border_segment: Don't expand the radius for very small dots. #3021

Merged
merged 1 commit into from Sep 7, 2018

Conversation

Projects
None yet
3 participants
@emilio
Member

emilio commented Sep 5, 2018

Otherwise we'd do antialiasing between the dots and it'd really look like a
solid line.

This is important for non-hidpi screens, for outlines and such in particular. I
can fix this also in the fragment shader after applying the other clips if you
think it's better, but this felt a bit less hacky.

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1434006.


This change is Reviewable

@emilio

This comment has been minimized.

Show comment
Hide comment
@emilio
Member

emilio commented Sep 5, 2018

r? @kvark or @gw3583

@kvark

kvark approved these changes Sep 5, 2018

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark
Member

kvark commented Sep 5, 2018

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 5, 2018

Contributor

📌 Commit fd74baa has been approved by kvark

Contributor

bors-servo commented Sep 5, 2018

📌 Commit fd74baa has been approved by kvark

bors-servo added a commit that referenced this pull request Sep 5, 2018

Auto merge of #3021 - servo:small-border, r=kvark
cs_border_segment: Don't expand the radius for very small dots.

Otherwise we'd do antialiasing between the dots and it'd really look like a
solid line.

This is important for non-hidpi screens, for outlines and such in particular. I
can fix this also in the fragment shader after applying the other clips if you
think it's better, but this felt a bit less hacky.

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1434006.

<!-- 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/3021)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 5, 2018

Contributor

⌛️ Testing commit fd74baa with merge f700cbe...

Contributor

bors-servo commented Sep 5, 2018

⌛️ Testing commit fd74baa with merge f700cbe...

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 5, 2018

Contributor

💔 Test failed - status-appveyor

Contributor

bors-servo commented Sep 5, 2018

💔 Test failed - status-appveyor

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark

kvark Sep 5, 2018

Member

thread 'main' panicked at 'size=1920×1080 ws=1916×1053', wrench\src\reftest.rs:496:9

We need to reduce the reftest image size

Member

kvark commented Sep 5, 2018

thread 'main' panicked at 'size=1920×1080 ws=1916×1053', wrench\src\reftest.rs:496:9

We need to reduce the reftest image size

@emilio

This comment has been minimized.

Show comment
Hide comment
@emilio

emilio Sep 5, 2018

Member

@bors-servo r=kvark

Thanks for the hint! I cropped it a bit.

Member

emilio commented Sep 5, 2018

@bors-servo r=kvark

Thanks for the hint! I cropped it a bit.

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 5, 2018

Contributor

📌 Commit ec08523 has been approved by kvark

Contributor

bors-servo commented Sep 5, 2018

📌 Commit ec08523 has been approved by kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 6, 2018

Contributor

⌛️ Testing commit ec08523 with merge a1610c3...

Contributor

bors-servo commented Sep 6, 2018

⌛️ Testing commit ec08523 with merge a1610c3...

bors-servo added a commit that referenced this pull request Sep 6, 2018

Auto merge of #3021 - servo:small-border, r=kvark
cs_border_segment: Don't expand the radius for very small dots.

Otherwise we'd do antialiasing between the dots and it'd really look like a
solid line.

This is important for non-hidpi screens, for outlines and such in particular. I
can fix this also in the fragment shader after applying the other clips if you
think it's better, but this felt a bit less hacky.

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1434006.

<!-- 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/3021)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 6, 2018

Contributor

💔 Test failed - status-appveyor

Contributor

bors-servo commented Sep 6, 2018

💔 Test failed - status-appveyor

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark

kvark Sep 6, 2018

Member

heh, did your crop blurred it? :)

REFTEST TEST-UNEXPECTED-FAIL | reftests\border\small-dotted-border.yaml == reftests\border\small-dotted-border.png | image comparison, max difference: 128, number of differing pixels: 196

Member

kvark commented Sep 6, 2018

heh, did your crop blurred it? :)

REFTEST TEST-UNEXPECTED-FAIL | reftests\border\small-dotted-border.yaml == reftests\border\small-dotted-border.png | image comparison, max difference: 128, number of differing pixels: 196

@emilio

This comment has been minimized.

Show comment
Hide comment
@emilio

emilio Sep 7, 2018

Member

Hmm, not really, in fact I even re-run the PNG command in (headless) wrench...

Member

emilio commented Sep 7, 2018

Hmm, not really, in fact I even re-run the PNG command in (headless) wrench...

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark

kvark Sep 7, 2018

Member

@emilio I assume reftests pass for you locally? (cd wrench && script/headless.py reftest)

Member

kvark commented Sep 7, 2018

@emilio I assume reftests pass for you locally? (cd wrench && script/headless.py reftest)

@emilio

This comment has been minimized.

Show comment
Hide comment
@emilio

emilio Sep 7, 2018

Member

Yes of course

Member

emilio commented Sep 7, 2018

Yes of course

@emilio

This comment has been minimized.

Show comment
Hide comment
@emilio

emilio Sep 7, 2018

Member

Well, to be honest there's a very small off-by-one in some text-related reftests. But I think those are not relevant.

Member

emilio commented Sep 7, 2018

Well, to be honest there's a very small off-by-one in some text-related reftests. But I think those are not relevant.

cs_border_segment: Don't expand the radius for very small dots.
Otherwise we'd do antialiasing between the dots and it'd really look like a
solid line.

This is important for non-hidpi screens, for outlines and such in particular. I
can fix this also in the fragment shader after applying the other clips if you
think it's better, but this felt a bit less hacky.

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1434006.
@emilio

This comment has been minimized.

Show comment
Hide comment
@emilio

emilio Sep 7, 2018

Member

@bors-servo r=kvark

  • Disabled the test on windows, as many other border reftests... Do we know why?
Member

emilio commented Sep 7, 2018

@bors-servo r=kvark

  • Disabled the test on windows, as many other border reftests... Do we know why?
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 7, 2018

Contributor

📌 Commit dea8c0e has been approved by kvark

Contributor

bors-servo commented Sep 7, 2018

📌 Commit dea8c0e has been approved by kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 7, 2018

Contributor

⌛️ Testing commit dea8c0e with merge bc452ec...

Contributor

bors-servo commented Sep 7, 2018

⌛️ Testing commit dea8c0e with merge bc452ec...

bors-servo added a commit that referenced this pull request Sep 7, 2018

Auto merge of #3021 - servo:small-border, r=kvark
cs_border_segment: Don't expand the radius for very small dots.

Otherwise we'd do antialiasing between the dots and it'd really look like a
solid line.

This is important for non-hidpi screens, for outlines and such in particular. I
can fix this also in the fragment shader after applying the other clips if you
think it's better, but this felt a bit less hacky.

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1434006.

<!-- 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/3021)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 7, 2018

Contributor

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

Contributor

bors-servo commented Sep 7, 2018

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

@bors-servo bors-servo merged commit dea8c0e into master Sep 7, 2018

5 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
Taskcluster (push) TaskGroup: success
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment