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

writeTextToCanvas treats "-" hyphen/minus, or "_" underscore, characters as space in some cases. #10649

Open
maxizrinUX opened this issue Aug 7, 2022 · 10 comments

Comments

@maxizrinUX
Copy link

writeTextToCanvas doesn't show anything for only hyphens or underscores, and also cuts off sometimes, but that seems to be another issue.
(Running Cesium JS in Electron, there's no cutoff, but all hyphen or underscore text still does not render.)

Text with mixed hyphens and underscores renders only hyphens.

Checking the return value, it seems canvas height is set to 0, when the string contains only hyphens or only underscores.

An HTML canvas works as a workaround.

Sandcastle example:
https://sandcastle.cesium.com/#c=vZVRa9swEMe/yuGXOmDJabvBaN1sLHsc7GFhT4Yg2+dETNYF6ezMG/3uk2NStq5rUkh6tsF30v+O31mSO+Wg07hFB3dgcQtz9Lpt5LddLM6jcufPybLSFl0eTW5zm9tRI9GyZo1eqqqKf+UWYEM+RMje7DPNlePwpuy1rB01n3DlEH08TWA6SQZFoY0pSLnqBnYZANIUFmvtwa9p6xMoWobgsdNNgxWQNT0MqYDXCAZrfj/KdKNW+FB36zTjAn/wgubKdsoHGPi/MW2EeGY8gCdjGV8qE8pcDd59iN2friNiKqfBrg81piL0YIl3HYKSnMOSTS+P7YMQFyLYwLRj+4sreQRWkvUMGNbHCRHfnh0RlsulgPG6eM2v9+7caIFL7J8DYKfDunwZlWJQxrwAKdjrsVydFUaI/eZ6dsmdlOjNIaJwl+Ecpbo+muMjMYdTdogeD1ObfkExTm6jJMo89wZnoxDgg2425BhaZ2IpU8ZmY1TASou2/I4sS++HZMPULP1TmlW6A13dPfE3gtIo78NI3RrzVf/EPJplaZj/j9SQqrRdfenQGdUP09aXs89jUEqZpcF9WslEplDuUebf

Browser:
Vivaldi 5.3.2679.70 (Stable channel) (64-bit)

Operating System:
Windows 10 Pro 21H2 64-bit

@ggetz
Copy link
Contributor

ggetz commented Aug 8, 2022

Thanks for the report @maxizrinUX. I can't seem to reproduce this on Chrome or Firefox. We do use some standard web APIs to implement writeTextToCanvas. Any chance this an underlying bug or difference of implementation in Vivaldi?

@maxizrinUX
Copy link
Author

Everything renders correctly in the Sandcastle?
The same issues persist for Vivaldi Android, that's 3 platforms now.
Screenshot attached.

Ignoring the text being cut off, that seems to be another issue.

There are 2 lines not rendering, one of all underscores, and another all hyphens, and lines with a mix of both don't render the underscores.

There's also a potentially undesired behavior, where the text is only cropped from the left for some reason.

Since this method is using an HTML canvas, and that works properly when manually created, I can only assume something is wrong with the method.

Screenshot_20220809-000608_Vivaldi Browser

@javagl
Copy link
Contributor

javagl commented Aug 15, 2022

When trying out the given sandcastle (in a version with red backgrounds, for debugging purposes), this is the result for me (on Chrome):

Cesium writeTextToCanvas A

The missing underscores and the cut-off text are... well, the same thing. It's cutting off a bit from the bottom. When there only was an underscore, nothing is left there...

Changing the call to measureText at

const dimensions = measureText(context2D, text, font, stroke, fill);
like this

  //const dimensions = measureText(context2D, text, font, stroke, fill);
  const metrics = context2D.measureText(text);
  const dimensions = {
    width: -metrics.actualBoundingBoxLeft + metrics.actualBoundingBoxRight,
    height: metrics.actualBoundingBoxDescent + metrics.actualBoundingBoxAscent,
    ascent: metrics.actualBoundingBoxAscent,
    descent: metrics.actualBoundingBoxDescent,
    minx: metrics.actualBoundingBoxLeft,
  };

results in the following:

Cesium writeTextToCanvas B

That looks about right.

I don't know what measureText is doing or what it is supposed to do, but it's obviously not doing the right thing. One could just go with removing that function, but there might be caveats to that: There is a line

  // Set canvas.dimensions to be accessed in LabelCollection
  canvas.dimensions = dimensions;

which might mean that these 'wrong' (?) dimensions are used somewhere else, and in the worst case, the 'wrong' results might already be anticipated there (so fixing this, pragmatically, could lead to problems elsewhere)


EDIT: Looking at the history, it seems like the copyright information was omitted when Source/ThirdParty/measureText.js was inlined in cb24b9b#diff-0af1c52dda6dc1bd380d142102be46c8710411063337d9af547d6baf01534159 (although this is unrelated to this issue.)

@ggetz
Copy link
Contributor

ggetz commented Aug 17, 2022

For context, looks like the function was rewritten in #9765.

(While it was "inspired by" the original third party implementation, it is not a direct in-lining. Though I would think we should have measureText mentioned in LICENCE.md by that logic.)

@maxizrinUX Would you be able to test before that PR and see if you're getting the same artifacts?

@maxizrinUX
Copy link
Author

Would you be able to test before that PR and see if you're getting the same artifacts?

I won't have the time in the foreseeable future.
We have deadlines approaching, so I made a workaround method, and forged on.
Sorry.

@javagl
Copy link
Contributor

javagl commented Aug 18, 2022

The state at https://github.com/CesiumGS/cesium/tree/22f7db308755cceaaddf33ea660d3b722baeb0ad (shortly before the linked PR was merged) creates this:

Cesium writeTextToCanvas old

It also omits the leading spaces in the first line, but the underscores are still there.

The state at https://github.com/CesiumGS/cesium/tree/5b777deb3dfa7411732ab55332e59edfc611485f (directly after merging the PR) creates the first image that I posted in my previous comment.

@GatorScott
Copy link

It seems that this function needs requirements clarification. Is it truly useful to clamp the canvas to the drawn text, which is effectively causing leading spaces to be ignored? Why isn't clamping occurring on the right side of text? What is the implementor supposed to do with the string parameter "baseline" (bottom of what)? Should this even be public or should it be made private?

@javagl
Copy link
Contributor

javagl commented Aug 22, 2022

The function will require some work, indeed. And as they say: "Pull requests are welcome". But displaying text is tremendously difficult.

(If somebody doesn't believe that: ꧁ خمن أي لغة هذه. ꧂ - and nowadays, some people even consider emojis as 'text' 😱 )

Some short notes:

Is it truly useful to clamp the canvas to the drawn text, which is effectively causing leading spaces to be ignored?

Clamping the canvas to the text does make sense and is useful. Otherwise, the question is: How large should the canvas be? (With the obvious answer: Well, as large as the drawn text...). The fact that the leading spaces are removed is (on a low technical level) unrelated to that, which is part of the answer to the next question:

Why isn't clamping occurring on the right side of text?

The wrong clamping appears to be the result of some attempts to compute the proper canvas size with the special measureText function. And apparently, this one has a few bugs: It properly computes the "right" end of the canvas, but does not properly compute the "left" side of the canvas. (And... also not the "bottom" of the canvas, FWIW...)

What is the implementor supposed to do with the string parameter "baseline" (bottom of what)?

This is just the textBaseline property for the canvas. If somebody doesn't know what to do with it, one should leave it as its default value. (But admittedly: I could imagine that using something else than the default value could cause glitches - text rendering is difficult...)

Should this even be public or should it be made private?

It is public and will remain public. Not only for backward compatibility, but also because it is a useful function - or even necessary in order to easily create billboards with text.


However, the image in #9765 showed some compatibility matrix where actualBoundingBoxAscend and actualBoundingBoxDescent had been marked as 'experimental', and not being supported by some browsers.

The latest version of that matrix is captured here (because it may change any minute, apparently):

Cesium TextMetrics

These properties - and more specifically, all properties that I used for the workaround in a previous comment - are no longer experimental, and supported by all browsers. So it might be possible to get rid of that custom measureText implementation.

@GatorScott
Copy link

@javagl thanks for the info. I should have been more specific as I was predominantly looking at measureText() rather than writeTextToCanvas(). Since I started looking at this last week I saw the actual... properties and was wondering what problem measureText() was trying to solve that built-in function wasn't providing.

@javagl
Copy link
Contributor

javagl commented Aug 22, 2022

OK, that clarifies ist. But measureText already is private (insofar that it is only a local helper function). What it does, exactly, is ... indeed obscure. Checking pixels to determine the bounding box looks dangerous, and I'd have to look more closely at the code (and maybe dump out the generated image as a PNG file to inspect it) to see what is going on there. But maybe it can just be removed and replaced with the built-in measureText function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Notable backlog items
Development

No branches or pull requests

4 participants