CesiumGS / cesium

An open-source JavaScript library for world-class 3D globes and maps :earth_americas:
https://cesium.com/cesiumjs/
Apache License 2.0
12.73k stars 3.45k forks source link

measureText problems with web fonts #378

Open shunter opened 11 years ago

shunter commented 11 years ago

Because web fonts are loaded asynchronously with the rest of the page, using them with labels introduces a potential race condition when we measure the size of the letters in order to make textures.

Both writeTextToCanvas and measureText internally create canvases, and set up the font information. If the font is a web font, I have already seen a case where this leads to an infinite loop inside measureText, which I assume results from the web font being loaded in between the canvas creations, resulting in different metrics which violate some of the assumptions in the algorithm.

Fixing this is going to be a major pain. Detecting if a web font has loaded or not seems to be a huge missing feature in web browsers, but I've found some libraries and sample code that might let us get it working.

http://www.lalit.org/lab/javascript-css-font-detect/ looks the most promising. The general idea is to use CSS fallback fonts and the assumption that the user's web font (whatever that is) will have a different width than the fallbacks, of course it fudges the widths more by making the font size really big and using the whole alphabet, etc.

The other thing to figure out is whether writeTextToCanvas needs to become asynchronous or not; it probably does since it can take an unknown amount of time to measure text correctly.

schmidtk commented 8 years ago

I encountered an infinite loop in Cesium caused by measureText when a label had a character comprised of two 16-bit character codes. You can reproduce by dropping the following into Sandcastle:

var viewer = new Cesium.Viewer('cesiumContainer');
viewer.entities.add({
    position : Cesium.Cartesian3.fromDegrees(0, 0),
    label : {
        text : String.fromCharCode(196, 129)
    }
});

Hit run, browser hangs. Recommend using Chrome so you can kill the tab easily.

schmidtk commented 8 years ago

@pjcozzi @shunter This is a pretty old issue so I'm not sure it will get visibility. This can easily reproduce an infinite loop when displaying unicode strings in labels. I encountered it loading a KML with non-English characters.

hpinkos commented 8 years ago

Thanks for reporting this @schmidtk. I can easily reproduce it with the code example you posted. It looks like it's getting stuck in this loop https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/ThirdParty/measureText.js#L173-L175

@shunter is this problem related to this issue or should I open a new issue to look into this?

schmidtk commented 8 years ago

I'm starting to wonder if the KML parser incorrectly interpreted the character as \xc4\x81 instead of \uc481. The latter would have treated it as a single character and probably wouldn't trigger the infinite loop. Unfortunately I'm out of the office until Monday so I don't have the KML available. I added a temporary workaround in my application to strip non-printable characters from labels, so it's not a pressing issue for me. Mostly wanted to report it since it freezes the browser. :)

shunter commented 8 years ago

Unrelated to the original issue. The loop in the code seems clearly wrong when the glyph renders blank, which is supposed to be checked by the whitespace regex at the top.

@schmidtk What character are you expecting to be using here?

String.fromCharCode(196, 129) is ā, but that's only from 196 (0xC4). 129 (0x81) is not defined. Is the character supposed to be \uC481 which is 쒁?

Fixing labels to work properly with unicode is likely to be a significant re-write, I suspect. see also #2543

schmidtk commented 8 years ago

I'll have to take a look at the KML again when I get back to the office on Monday. I didn't have the time to thoroughly debug and used the workaround to strip blank characters from the text instead. It may be expecting an Arabic font (the label was for Baghdad), which is why I thought it may be semi-related to this issue.