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

Only draw vertical and horizontal edge when border radius is less then border width #1975

Merged
merged 1 commit into from Nov 3, 2017

Conversation

@subsevenx2001
Copy link
Contributor

subsevenx2001 commented Nov 1, 2017

#1805 is caused by fragment shader incorrectly processes corner pixels outside the clip reigon which is originally written for handling border width > border radius case.

This patch checks whether border radius is less than border width or not before drawing pixel.


This change is Reviewable

@glennw
Copy link
Member

glennw commented Nov 1, 2017

The CI is failing with these tests:

Reftests with unexpected results:

	reftests/border/border-radii.yaml == reftests/border/border-radii.png

	reftests/border/border-suite.yaml == reftests/border/border-suite.png

	reftests/border/border-suite-2.yaml == reftests/border/border-suite-2.png

	reftests/border/border-suite-3.yaml == reftests/border/border-suite-3.png

	reftests/border/degenerate-curve.yaml == reftests/border/degenerate-curve.png

It's possible they might just need to be re-generated, depending on what the differences are. You can run the tests locally with ./headless.py reftest >ref.log and open the log in the reftest analyzer.

@subsevenx2001 subsevenx2001 force-pushed the subsevenx2001:bug1404158 branch 2 times, most recently from 460c5f6 to c881343 Nov 2, 2017
@subsevenx2001 subsevenx2001 reopened this Nov 2, 2017
@subsevenx2001
Copy link
Contributor Author

subsevenx2001 commented Nov 2, 2017

screen shot 2017-11-02 at 2 55 50 pm

I ran reftest locally and corrected some logic, now this one is the only failed reftest.

But the difference is so little that I cannot see it unless with "Circle the Difference" checked.

Hi @glennw , is that the case which need ref image being regenerated?

@glennw
Copy link
Member

glennw commented Nov 2, 2017

In that case, you can run ./headless.py png <reftest name> and it will generate and crop a new baseline image.

@glennw
Copy link
Member

glennw commented Nov 2, 2017

@subsevenx2001 Is there any way we can add a new reftest that covers this change, to ensure we don't regress in the future?

@subsevenx2001
Copy link
Contributor Author

subsevenx2001 commented Nov 2, 2017

@glennw It seems that the bug itself only triggered in certain zoom ratio, and the ratio varies among different screen resolution, I am not sure how to trigger it with reftest since we don't have screen resolution in headless mode when performing reftest.

@subsevenx2001 subsevenx2001 force-pushed the subsevenx2001:bug1404158 branch from c881343 to 081becd Nov 2, 2017
@glennw
Copy link
Member

glennw commented Nov 2, 2017

@subsevenx2001 Yea, it could be tricky. I think I ported the test case at https://bug1404158.bmoattachments.org/attachment.cgi?id=8913468 to a wrench yaml file and was able to reproduce it when I was doing some testing. I've lost that file, but it's probably worth just converting that test case to a wrench YAML file and seeing if it reproduces.

@subsevenx2001
Copy link
Contributor Author

subsevenx2001 commented Nov 2, 2017

@glennw I see 1920x1080 when running the script generating reference image.

I think maybe it is the base resolution when running headless mode, perhaps we can try reproducing it on a 1080p screen and convert it in YAML.

…us is less then border width
@subsevenx2001 subsevenx2001 force-pushed the subsevenx2001:bug1404158 branch from 081becd to a1cba79 Nov 3, 2017
@subsevenx2001
Copy link
Contributor Author

subsevenx2001 commented Nov 3, 2017

Add a reftest for testing bogus line in border radius.

@glennw
Copy link
Member

glennw commented Nov 3, 2017

I submitted a gecko try run for this:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a374b822433151bd6b29fabb26b78d7fe649cfc9

If it's all green, then this is good to merge.

@glennw
Copy link
Member

glennw commented Nov 3, 2017

Try looks good, thanks!

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2017

📌 Commit a1cba79 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Nov 3, 2017

Testing commit a1cba79 with merge fae962b...

bors-servo added a commit that referenced this pull request Nov 3, 2017
Only draw vertical and horizontal edge when border radius is less then border width

#1805 is caused by fragment shader incorrectly processes corner pixels outside the clip reigon which is originally written for handling border width > border radius case.

This patch checks whether border radius is less than border width or not before drawing pixel.

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

bors-servo commented Nov 3, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: glennw
Pushing fae962b to master...

@bors-servo bors-servo merged commit a1cba79 into servo:master Nov 3, 2017
4 checks passed
4 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@subsevenx2001 subsevenx2001 deleted the subsevenx2001:bug1404158 branch Nov 3, 2017
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.