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

Remove custom measure text #11747

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Remove custom measure text #11747

wants to merge 3 commits into from

Conversation

javagl
Copy link
Contributor

@javagl javagl commented Jan 7, 2024

Description

The writeTextToCanvas function writes text into a canvas, basically to create "an image of the text". This is used for labels and Billboards. The size of this canvas is computed with a custom function that was supposed to serve a smilar purpose as measureText. This custom function has some issues. The reason for using the custom implementation was probably that certain properties of the TextMetrics that are required by writeTextToCanvas had not been supported on all browsers. As mentioned in a related comment, the TextMetrics browser compatibility summary suggests that all required properties are now widely available. So the custom implementation can be replaced by the built-in measureText function.

Issue number and link

Fixed issues:

Related issues:

Caused issues:

  • It will certainly cause some issues. We'll see...

Testing plan

Until now, I only tested it visually. The following is a sandcastle that is based on #10649 , with some additional cases:

https://sandcastle.cesium.com/index.html#c=zZbbattAEIZfZdBNZLDWpx5dx7R1Cr1oKaSmvRGYlTRWlqx3ze7Krhv8DoWQi0IP1+0L6XU6smK3pCkB2S4eyUYzO/9ovrWEZ8YNzATO0cAxKJzDAK3IJuzdKuaHXrzyB1o5LhSa0Ks9CVWpYKNy8b1IUnTkGUyE26Qy6xYSWSLsVPIFlQ89pRWGHhXYlEDlhBNoGU8S/yJUAFNtKaJVd93KgBtHV1x12NjoyQmmBtH6zTo0a/VCEQkpI81N0oVVBQAx4SluCsyNcDjED26oB1zNuPXLLKCO4IY5PQ0C+LeFXn0tvoCIx+ep0ZlKBlpq87vjwmOnL05gWSbXrkU25pL6aq/cJX0vazvbjKDJmmSdynviBcFRQEaIaw3cjXhNtrwLMdbKOkB6DHYI+2ALWBiNRgGUx9F+mHdI+rg6KWEG688+OHdH2WpvQ0l24Hj3t3k39/dq7pDwUXXC59o5PYEieuiY7VZ1zFORnrnA6UDi2HXhNbeO/nv9/FN+mV/l32tdeIlSash/5l/yb/nn/BJ867KE2qa1/AdFr/KvB79D97Z4EFDqObgzYYFODnbKYwSt5OLgqR9Wp4ZDh+tUn7a8Z5Ge4f//ScdyMdQ+FjGv7vVW82h/fbOnYjLVxkFmpM9Yw+GExlTagUaUxec0zsbWFsIitdf4U9pLxAxEcnzLbAyx5NbSyjiT8q34SMNuv9eg/L+kUvNEqPTNDA0Nx0XaWav/qgwyxnoNcm9XOq1lxM2Nyr8A

The comparison of the current state and the state after this PR is shown here:

Cesium MeasureText Test

It shows that the missing underlines and the missing (?) leading spaces are now included.

The following is a comparison of the current state and the new state for the example from #11705 :

Cesium MeasureText Comparison 11705

It shows that the background is no longer longer than the rendered text.

The following is a comparison of the output of the "Labels" Sandcastle for the old and the new state:

Cesium MeasureText Comparison

It shows that there are small differences in the positioning, but ... it's nearly impossible to say what is "right" or "wrong" here. The only thing that bugs me a bit is that in some cases, it looks like the text is not properly aligned to the baseline:

Cesium MeasureText Alignment

After looking at the approach of the LabelCollection and its "Glyph-based" rendering, I wonder whether this can be avoided in all cases. (Subjectively, in other cases, the text appears less jagged with the new state, so...)

Beyond that, I'm reasonably sure that some of the automated tests will fail with the new state, e.g. some not-really-but-somehow pixel-exact check in writeTextToCanvasSpec or so. But this will have to be investigated. Some tests can probably be fixed pragmatically by adjusting them to "the new truth"...

Author checklist

  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

@ggetz
Copy link
Contributor

ggetz commented Jan 23, 2024

Thanks @javagl! Great to removal of redundant code.

it looks like the text is not properly aligned to the baseline

I believe the baseline issue is unrelated to this change. Rather, this is caused by quantization error as described in #8474.

Is this PR ready for official review?

@javagl
Copy link
Contributor Author

javagl commented Jan 23, 2024

One of the reasons why this is still a "draft" is that there's still a

TODO See if baseline jagging can be avoided

in my local TODO list. I'll read the linked issue(s) and see whether the change here did really make it "worse", or whether it is now (equally 'bad' but) only 'slightly different'. A single pixel can be a lot for text...

@javagl
Copy link
Contributor Author

javagl commented Jan 23, 2024

Based on a quick comparison with the sandcastle in the linked issue, the jagging seems to have become worse for small fonts:

Cesium Jagged Baseline

It is not "terrible", and only happens for fonts that are ... well, nearly too small to read them anyhow, but ... I'd still like to investigate that further (with the goal to either 1. not make it worse or 2. (preferred) make it better), but I'll have to zoom into the exact place where the glyphs are generated, look at the exact differences between the old and the new function, and try to figure out where this difference comes from.

@javagl
Copy link
Contributor Author

javagl commented Jan 24, 2024

I did zoom a little bit into that, but don't have a perfect solution (yet?). The following is rather a few "notes" to keep track of the considerations here.

The most critical part of the "metrics" that are computed for the text are the height and the descent. The original measureText function relied on "counting filled pixels", which is brittle in some way. The updated measureText functionality derived the required information from the TextMetrics, using TextMetrics.actualBoundingBoxDescent and similar properties, which solved the main part of #10649

But both approaches cause "jagged" lines in some cases.

The height/ascend properties are used for "assembling" the actual label text, out of individual glyphs. The positioning of the glyphs is computed by mushing together the height of each glyph and a value that was computed from all glyphs, (Details are omitted here. Reverse-engineering the details of LabelCollection#repositionAllGlyphs could be a challenge...)

For a combination of letters like x, and d (ascender), and p (descender), there is no common "height" or "y-value" that can reliably be computed from the bounds of individual letters.

This can be solved, to some extent, by not using the actualBoundingBoxDescent, but the fontBoundingBoxDescent. This is a "global" property for the font that is used for rendering. And this actually improves things a lot. Here is an example of the current state, and the state that uses the font... family of properties:

Cesium Jagged Baseline No More

The current one looks jagged. The new one looks nice and clean.

But... as the name suggests, the fontBoundingBoxDescent property does not take into account the actual bounds. This means that some of the ~"more unusual" characters, like ꧁Javanese rerenggans꧂ or combined unicode characters like À̴̴̴̴̴̖̖̖̖̖̀̀̀̀ are not handled properly. It can be seen in this sandcastle (linked below) that all other cases work well (and there is no jagging for smaller fonts), but these "unusual" characters are not displayed properly, because the actual.... height is larger than the font... height:

Cesium Font Bounds

Rough ideas for addressing this:

  • One could slam in some height = max(font..., actual....) and hope that this works. But this is unlikely, because it will re-introduce the problem of glyphs having different heights, which caused the "jagged" look
  • One could try to compute the actual bounds of the whole text, and use this as the size for the final label. But this would require a (complete understanding and) probably large rewrite/restructuring of the whole "glyph-based" rendering approach

Maybe there's an intermediate solution. My gut feeling is that this maxGlyphY could be the crux here. But that has to be investigated further.


The test sandcastle:

http://localhost:8080/Apps/Sandcastle/#c=1ZfNattAEMdfZdAlMliyHadfrhOaOoVSWgppaC8Cs5LG8pLVrtldyXaDoe0TFEIKhX4cQ3vuY+Sud8hTdGzHaUkTCnZS1LUlPLPzH89vJdk7OdOQcxyihk2QOIQOGp6l/suZzw2caGZ3lLSMS9SBU7kfyLnC784nX/E4QUuWxpjb81Df2LFAP+ZmINiY0geOVBIDhxKcp0BpueVofBbH7kEgAQbKkEfJ1qKUDtOWPjHZ9HtapTuYaETj1qtQr1SnipALESqm4xbMMgDwlCV4nmCoucU9HNk91WEyZ8adRwFVBBeGVQPPg6tH4FQX4gMIWbSfaJXJuKOE0r8qnlr+7qMdmMyDK2ciEzFBda3PzAmdJ5VrWwyv7tfrzaVXxPG8NY8GAS408HfAM67JPwO8vQIgdLtdD+avtZJz3luekyC9xXETlNfF2FhfhZFGqeFurfIklv9BbNxdnu+hslalMPWWG3K9sTzkLk/61rPKE9izLXjGjKX/WLd4XxwWR8XXSgseoxAKiu/Fp+JL8bE4BNfYLKaiaa74Rt6j4nPJ12djhZsAhRqC7XMD9GZgBixCUFKMS858Z3lmKDdac/n9lLMdqhz/w8vZXGHHdHr89gnLmUSDoFGjTBImzenxu5Ijr7CHgu2TNyc/Tj5cdQYJOGLpQCD0lIZM8kjFCJFKQ2oJYoj6TLOIfglNuddoY4W9yehG0XpivKfcC5zTCKfqtGcd19biqx/wdKC0hUwL1/drFum6MIKuhVm0Tw1bZGbCaWi79ru0HfMceLx5SfcHkWDG0EwvE+IFf03t3Fa7RvF/SIViMZfJ8xw1tX/TsH5j6+nc6ft+u0bm5UqrlAiZvpD5Jw

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

2 participants