-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Remove custom measure text #11747
Conversation
One of the reasons why this is still a "draft" is that there's still a
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... |
Based on a quick comparison with the sandcastle in the linked issue, the jagging seems to have become worse for small fonts: 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. |
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 But both approaches cause "jagged" lines in some cases. The For a combination of letters like This can be solved, to some extent, by not using the The current one looks jagged. The new one looks nice and clean. But... as the name suggests, the Rough ideas for addressing this:
Maybe there's an intermediate solution. My gut feeling is that this The test sandcastle: |
I think that this is important. And the current state of this PR already is an improvement (at least, as long as the text does not use things like ꧁Javanese rerenggans꧂ ). But I'll (have to) close it, until I have the chance to properly address the more subtle points as well. |
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 asmeasureText
. This custom function has some issues. The reason for using the custom implementation was probably that certain properties of theTextMetrics
that are required bywriteTextToCanvas
had not been supported on all browsers. As mentioned in a related comment, theTextMetrics
browser compatibility summary suggests that all required properties are now widely available. So the custom implementation can be replaced by the built-inmeasureText
function.Issue number and link
Fixed issues:
writeTextToCanvas
andmeasureText
function #9767Related issues:
Caused issues:
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:
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 :
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:
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:
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
CHANGES.md
with a short summary of my change