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

Text baseline alignment issues #1562

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

asturur
Copy link
Contributor

@asturur asturur commented Apr 25, 2020

This PR tries to improve the precision with which different text baselines are calculated.
With node-canvas the alphabetic and bottom baseline is always perfect, it works good.

top, middle, ideographic and hanging are a bit off.

The tweak is a 2 part change.
In the function getBaselineAdjustment we copy the pangoLayout and we swap the text with a string that uses all the extent of the selected font, this improve automatically the middle use case.

Then a correction factor is applied to the positions that do not seem correct, this correction factor is found with trial and error.

AFTER FIX

FIREFOX

image
image
image

CHROME

image
image
image

BEFORE FIX

FIREFOX

image
image
image

CHROME

image
image
image

  • Have you updated CHANGELOG.md?

@asturur
Copy link
Contributor Author

asturur commented Apr 25, 2020

I also extended the tests with differenf fonts, to show this fix CAN'T work perfectly with all fonts ( middle seems an improvement anyway );

So this is the fix with the different text string use for measurement, with NO correction factor:
image

and this is with ALSO the correction factor
image

@@ -947,7 +947,7 @@ describe('Canvas', function () {
assert.ok(metrics.alphabeticBaseline > 0) // ~4-5
assert.ok(metrics.actualBoundingBoxAscent > 0)
// On the baseline or slightly above
assert.ok(metrics.actualBoundingBoxDescent <= 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tweaked the baseline offset, so i expect some text measurement to change slightly. Unsure if this is ok or not. Any thought anyone?

cairo_matrix_t matrix;
cairo_get_matrix(ctx, &matrix);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matrix does not seem to be used in the function. Is cairo_get_matrix having some side effect that is necessary for the code below to work?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it is used for alphabeticBaseline below

@asturur
Copy link
Contributor Author

asturur commented Apr 27, 2020

@zbjornson @LinusU any comment / recomendation?

@LinusU
Copy link
Collaborator

LinusU commented Apr 27, 2020

This is way outside my area of expertise 😅

I'm always a bit afraid of magic numbers, but since this clearly makes it better I think it's a good change 👍

Hopefully @zbjornson & @chearon will have some better input 😄

@chearon
Copy link
Collaborator

chearon commented Apr 27, 2020

You should be able to get the right X-height via pango_font_metrics_get_strikethrough_position (for the "middle" baseline).

For the other ones I guess some kind of hack is reasonable. I don't quite understand it yet and I'll try to give a fuller review later. Ideally we would extract the FreeType face and look at ideographic/hanging/etc from the font tables. But Pango doesn't use FreeType on macOS/Windows so there'd have to be platform-specific code. More reasons I should remove Pango and use lower-level libraries...

@asturur
Copy link
Contributor Author

asturur commented Apr 27, 2020

i m willing to write more code if it does not hinder performances.
Totally looking at pango_font_metrics_get_striketrough_position asap and remove the different text hack i made if it works, definitely a cleaner approach.

@asturur
Copy link
Contributor Author

asturur commented Jul 6, 2020

sorry i forgot about this PR completly.

@LinusU
Copy link
Collaborator

LinusU commented Jul 6, 2020

@asturur no worries, if you feel that you don't have time to improve further, I'm open to merging it as is! Thank you for spending time on this project ☺️

@asturur
Copy link
Contributor Author

asturur commented Jul 6, 2020

I'll give a stab tomorrow!

@asturur
Copy link
Contributor Author

asturur commented Jul 18, 2020

@charon, i tried this:

  PangoRectangle logical_rect;
  PangoLayout* measureLayout = pango_layout_copy(layout);
  PangoFontMetrics *metrics;
  pango_layout_set_text(measureLayout, "gjĮ測試ÅÊ", -1);
  pango_layout_line_get_extents(pango_layout_get_line(measureLayout, 0), NULL, &logical_rect);
  metrics = PANGO_LAYOUT_GET_METRICS(measureLayout);
  int strike_thickness = pango_font_metrics_get_strikethrough_thickness(metrics);
  int strike_position = pango_font_metrics_get_strikethrough_position(metrics);
  double scale = 1.0 / PANGO_SCALE;
  double ascent = scale * pango_layout_get_baseline(measureLayout);
  double descent = scale * logical_rect.height - ascent;
  double middle = ascent - (scale * strike_position) + (scale * strike_thickness / 2);
  // 0.072 is a constant that has been chosen comparing the canvas output
  // if some code change, this constant can be changed too to keep results aligned
  double correction_factor = scale * logical_rect.height * 0.072;

EDIT: tested on the macbook where i developed the code, this seems good:

firefox:
image
chrome:
image

approach before my changes:
image

my new macbook cannot compile node-canvas correctly, i get the text scaling bug, node12, catalina 10.15.5
I wonder how to solve it.

@asturur
Copy link
Contributor Author

asturur commented Jul 18, 2020

no as not said.
In the case in which the context is vertically scaled this thing breaks, Probably the striketrhoug position contains the scale factor of the context inside, and i have no idea how to revert it.

@asturur
Copy link
Contributor Author

asturur commented Jul 18, 2020

The tests aren't passing anymore. While before they passed. What could it be? is something changed in tests? I reverted and push forced to the passing commit.

@chearon
Copy link
Collaborator

chearon commented Jul 20, 2020

So this PR should be updated to use pango_font_metrics_get_strikethrough_position for middle baseline, right?

(edit: and not sure about the tests, sorry!)

@asturur
Copy link
Contributor Author

asturur commented Jul 20, 2020

i did update it, but the results with scaling where compleltely off, so i reverted back.

  PangoRectangle logical_rect;
  PangoLayout* measureLayout = pango_layout_copy(layout);
  PangoFontMetrics *metrics;
  pango_layout_set_text(measureLayout, "gjĮ測試ÅÊ", -1);
  pango_layout_line_get_extents(pango_layout_get_line(measureLayout, 0), NULL, &logical_rect);
  metrics = PANGO_LAYOUT_GET_METRICS(measureLayout);
  int strike_thickness = pango_font_metrics_get_strikethrough_thickness(metrics);
  int strike_position = pango_font_metrics_get_strikethrough_position(metrics);
  double scale = 1.0 / PANGO_SCALE;
  double ascent = scale * pango_layout_get_baseline(measureLayout);
  double descent = scale * logical_rect.height - ascent;
  double middle = ascent - (scale * strike_position) + (scale * strike_thickness / 2);
  // 0.072 is a constant that has been chosen comparing the canvas output
  // if some code change, this constant can be changed too to keep results aligned
  double correction_factor = scale * logical_rect.height * 0.072;

The result was OK for middle when used at scale 1, but bad when used with any other scaling.
So i removed it.

@BlueGhostAlt
Copy link

Any updates on this? This is an issue that I'd really like to see fixed.

@asturur
Copy link
Contributor Author

asturur commented Oct 11, 2020

i did not have time to investigate why the test started failing, i can try again rebasing the pr on a fresh branch

@asturur
Copy link
Contributor Author

asturur commented Oct 11, 2020

This was a minor fix, wasn't a final solution for 1:1 match with canvas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants