Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upclip text against the local clip rect after snapping rather than before #3048
Conversation
|
complete! all files reviewed, all discussions resolved
|
@lsalzman Is there any way we can have reftest(s) for this - it'd be good to be able to pick up any regressions on this stuff in WR, where we can run tests with subpixel on/off on each platform etc. |
|
Added a reftest |
|
@lsalzman just to clarify, the test is failing without your changes, right? |
Of course |
|
Did suggested cleanups. |
|
Thanks, shipit! |
|
|
clip text against the local clip rect after snapping rather than before This partially fixes Gecko bug https://bugzilla.mozilla.org/show_bug.cgi?id=1477625 Previously, we took the glyph rect within the atlas and then added the glyph offset with subpixel offset and all onto it, and critically, clipped it first, then snapped it second. The problem is the glyph rect in the atlas represents a conservative bounding box of the glyph. So if you add an offset to it, which might cause it to go outside the clip and snip it. Then the snapping pushes it back inside the bounds, so that the snipped portion is actually still inside the clip, leaving the glyph wrongly snipped. The way to fix this, while not quite as pretty as the previous code, is to ensure that we snap the glyph rect into place in device space first, then transform it back to local space to perform the clipping. This way, snapping is first, clipping is second. I've managed to keep the amount of math comparable for performance, and maybe the snapping code is even a bit simpler than before due to necessarily snapping the glyph rect origin rather than individual corners. <!-- 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/3048) <!-- Reviewable:end -->
|
|
lsalzman commentedSep 12, 2018
•
edited by larsbergstrom
This partially fixes Gecko bug https://bugzilla.mozilla.org/show_bug.cgi?id=1477625
Previously, we took the glyph rect within the atlas and then added the glyph offset with subpixel offset and all onto it, and critically, clipped it first, then snapped it second.
The problem is the glyph rect in the atlas represents a conservative bounding box of the glyph. So if you add an offset to it, which might cause it to go outside the clip and snip it. Then the snapping pushes it back inside the bounds, so that the snipped portion is actually still inside the clip, leaving the glyph wrongly snipped.
The way to fix this, while not quite as pretty as the previous code, is to ensure that we snap the glyph rect into place in device space first, then transform it back to local space to perform the clipping. This way, snapping is first, clipping is second. I've managed to keep the amount of math comparable for performance, and maybe the snapping code is even a bit simpler than before due to necessarily snapping the glyph rect origin rather than individual corners.
This change is