Skip to content
Snippets Groups Projects

win32u: Fix rendering of the rotated and scaled text.

Open Dmitry Timoshkov requested to merge dmitry/wine:win32u into master
3 unresolved threads

These patches make a test case attached to the bug https://bugs.winehq.org/show_bug.cgi?id=33190 work.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
250 246 }
251 247
252 248 /* scale width and height but don't mirror them */
253 249
254 250 static inline INT width_to_LP( DC *dc, INT width )
255 251 {
256 return GDI_ROUND( (double)width * fabs( dc->xformVport2World.eM11 ));
252 return INTERNAL_XDSTOWS( dc, width );
257 253 }
258 254
259 255 static inline INT height_to_LP( DC *dc, INT height )
260 256 {
261 return GDI_ROUND( (double)height * fabs( dc->xformVport2World.eM22 ));
257 return INTERNAL_YDSTOWS( dc, height );
258 }
259
  • Developer

    So now INTERNAL_XDSTOWS() and width_to_LP() are the same. If this is really what we want we should unify them before changing the implementation.

    Also, hypot() may be useful here.

  • Author Developer

    Hi Huw,

    thanks for the review.

    /* scale width and height but don't mirror them */

    static inline INT width_to_LP( DC *dc, INT width ) {

    • return GDI_ROUND( (double)width * fabs( dc->xformVport2World.eM11 ));
    • return INTERNAL_XDSTOWS( dc, width ); }

    static inline INT height_to_LP( DC *dc, INT height ) {

    • return GDI_ROUND( (double)height * fabs( dc->xformVport2World.eM22 ));
    • return INTERNAL_YDSTOWS( dc, height ); +}

    So now INTERNAL_XDSTOWS() and width_to_LP() are the same. If this is really what we want we should unify them before changing the implementation.

    Do you mean by unifying adding a patch that replaces width_to_LP() by INTERNAL_XDSTOWS() and height_to_LP() by INTERNAL_YDSTOWS()? If yes, then with current implementation this leads to test failures.

    Also, hypot() may be useful here.

    Thanks.

  • Dmitry Timoshkov changed this line in version 2 of the diff

    changed this line in version 2 of the diff

  • Author Developer

    So now INTERNAL_XDSTOWS() and width_to_LP() are the same. If this is really what we want we should unify them before changing the implementation.

    Also, hypot() may be useful here.

    New version of the patches should follow the suggestions.

  • Please register or sign in to reply
  • Dmitry Timoshkov added 672 commits

    added 672 commits

    • 209c75b1...174bb777 - 670 commits from branch wine:master
    • 6c8f9f11 - win32u: Fix device<->world width/height converters.
    • 70fd2ace - win32u: NtGdiExtTextOutW() should translate x,y from logical to device units at the last step.

    Compare with previous version

  • Author Developer

    Is there anything else that could be improved to make the patches accepted?

  • Author Developer

    Is there any chance for a review?

    • Author Developer

      The patches from this MR are included in wine-staging since 18 Oct 2024, does this qualify as an extented testing timeframe? What else is needed to get these patches reviewed?

  • Dmitry Timoshkov left review comments

    left review comments

  • Sorry about the delay.

    It seems to me we could have some tests for this. e.g. create a font with an identity world transform, measure its metrics and some of its glyph widths, then add a transform and use the identity metrics/widths to compute the expected metrics/widths based on the new transform.

    Also, if we're going to do some function renaming, could we go from e.g. INTERNAL_YDSTOWS to the vastly more readable height_to_LP rather than the other way around? Could we please do this renaming in a separate commit, so that the actual changes end up being in a smaller commit?

    Edited by Huw Davies
  • Author Developer

    Thanks for the review. I considered adding some tests, however pretty soon figured out that Wine doesn't have a metrics compatible scaleable font with Windows, our Tahoma has pretty different metrics, and that makes the tests meaningless. So, instead I used a test application attached to the mentioned in the description bug report.

    Using height_to_LP instead of INTERNAL_YDSTOWS sounds reasonable.

    • I considered adding some tests, however pretty soon figured out that Wine doesn't have a metrics compatible scaleable font with Windows, our Tahoma has pretty different metrics, and that makes the tests meaningless.

      Did you try calculating the expected metrics from the metrics obtained without a world transform?

    • Author Developer

      You mean using something like height_to_LP() in the tests? If yes, what kind of benefit would it provide to simple comparisons of expected values?

    • Yes. It would allow for the variation in metrics returned by Windows / Wine in the non-transformed case and basically test that your scaling works as Windows does.

    • Author Developer

      Font scaling in Wine already works pretty much similar to Windows. The problem is with glyph positioning during rotation, and I have no idea how to test that.

    • Perhaps I'm misunderstanding the problem; I wasn't asking about the glyph positions, just the font metrics and glyph widths.

    • Author Developer

      There are existing tests for outline text metrics and world transforms, and this MR shouldn't influence that. What kind of new tests would like to be added?

    • Developer

      There are existing tests for outline text metrics and world transforms, and this MR shouldn't influence that. What kind of new tests would like to be added?

      Ones that show that your proposed changes are needed.

    • Author Developer

      Does new version of the patches meet your suggestions?

    • Please register or sign in to reply
  • Dmitry Timoshkov added 6306 commits

    added 6306 commits

    • 70fd2ace...f3843ea1 - 6301 commits from branch wine:master
    • 3435d156 - win32u: Use correct helper for converting width to device units.
    • 0c28e56a - win32u: Use slightly more readable names for DP/LP converters.
    • 4bdfcc54 - gdi32/tests: Add some tests for rotated font metrics.
    • 3fa748cc - win32u: Fix device<->world width/height converters.
    • 18668136 - win32u: NtGdiExtTextOutW() should translate x,y from logical to device units at the last step.

    Compare with previous version

  • Author Developer

    Is there anything else that could be added or improved here?

  • Please register or sign in to reply
    Loading