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 DirectWrite raster_bounds arithmetic #92

Merged
merged 2 commits into from Jul 26, 2019

Conversation

@idursun
Copy link
Contributor

idursun commented Jul 25, 2019

Fixes #85

  • raster_bounds was returning a rectangle at origin (0,0) whereas the origin of the bounds rectangle returned by get_alpha_texture_bounds is not (0,0). This caused offsetting origin calculation to be off when passed to rasterize_glyph which caused the clipping.

Offsetting origin calculation that I am referring to is:

https://github.com/pcwalton/font-kit/blob/6801dce15cbc75448d99420eeebbe8c867585f36/examples/render-glyph.rs#L133-L136

This change sets the origin of the raster_bounds rectangle so that the origin offset arithmetic actually holds for directwrite.

c:\repositories\rust\font-kit>cargo run --example render-glyph ArialMT { 32
    Finished dev [unoptimized + debuginfo] target(s) in 0.09s
     Running `target\debug\examples\render-glyph.exe ArialMT { 32`
glyph 94:
            ▓▓██████
          ██████████
        ▒▒████▒▒
        ▓▓██▓▓
        ▓▓██▓▓
        ▓▓██▓▓
        ▓▓██▓▓
        ▓▓██▓▓
        ▓▓██▓▓
        ████▓▓
        ████▒▒
      ▒▒████▒▒
    ▒▒████▓▓
░░████████
░░████▒▒
░░████████
    ▒▒████▓▓
      ▒▒████▒▒
      ░░████▒▒
        ████▓▓
        ▓▓██▓▓
        ▓▓██▓▓
        ▓▓██▓▓
        ▓▓██▓▓
        ▓▓██▓▓
        ▓▓██▓▓
        ▒▒████▒▒
          ██████████
            ▓▓██████
@RazrFalcon
Copy link
Collaborator

RazrFalcon commented Jul 26, 2019

I've tested it with silicon and looks like everything is fine. Could you add some tests, especially for the { case?

@idursun
Copy link
Contributor Author

idursun commented Jul 26, 2019

Sure 👍

Similar to the rasterization tests (i.e. rasterize_glyph_with_grayscale_aa, rasterize_glyph_bilevel) that are asserted with check_L_shape?

@RazrFalcon
Copy link
Collaborator

RazrFalcon commented Jul 26, 2019

Yes, I think this will work well.

@idursun
Copy link
Contributor Author

idursun commented Jul 26, 2019

I have added a "best-effort" test for verifying the rasterization of any top-down symmetrical letters.

Currently, it's only testing { letter and running against windows and mac. Although, I believe it should kinda work against all loaders.

@RazrFalcon
Copy link
Collaborator

RazrFalcon commented Jul 26, 2019

I think this is good enough. Thanks again!

Only testing '{' character at the moment
@idursun idursun force-pushed the idursun:fix-dwrite-raster-bounds branch from 31d59c6 to 4b84d5e Jul 26, 2019
@idursun
Copy link
Contributor Author

idursun commented Jul 26, 2019

Changed to target only Windows. Apparently, macos glyph is not as symmetrical.

@RazrFalcon RazrFalcon merged commit 2ed8371 into servo:master Jul 26, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@idursun idursun deleted the idursun:fix-dwrite-raster-bounds branch Jul 26, 2019
@pcwalton
Copy link
Collaborator

pcwalton commented Jul 26, 2019

Thanks!

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.

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