-
Notifications
You must be signed in to change notification settings - Fork 3.7k
refactor(labels): Remove custom measureText implementation #13081
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for the pull request, @donmccurdy! ✅ We can confirm we have a CLA on file for you. |
bcdfc8d to
558a4e3
Compare
558a4e3 to
45027d3
Compare
|
@javagl would you mind reviewing this one? 🙏 |
|
First getting rid of the custom implementation (and ensuring that this does not change anything else) sounds like a plan. Hopefully, the related issues (e.g. hyphen/underscore handling) can then still be resolved based on that state. I'll need a short refresher for the last experiments that I did there - that back and forth of |
|
That Now, it has a I started some review/test, but may have to wrap it up later. First, regarding the test that you posted: I wanted to try this out, and added the following as a spec file (Just copying both Nearly all the tests are failing with
Before investigating it further: Is this how this (temporary, local) test is supposed to be run? (I said that nearly all the tests are failing. The 'spanish' ones are passing. I don't expect anyone to make this make sense...) I ran a test similar to the one from #11747 (comment) , using this sandcastle const viewer = new Cesium.Viewer("cesiumContainer");
viewer._cesiumWidget._creditContainer.style.display = "none";
viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(-75.1641667, 39.9522222),
label: {
text: "Philadelphia",
font: "12px Helvetica",
fillColor: Cesium.Color.SKYBLUE,
outlineColor: Cesium.Color.BLACK,
outlineWidth: 2,
style: Cesium.LabelStyle.FILL_AND_OUTLINE,
},
});And I wanted to post a GIF showing the differences. But there are none (at least, none that are visible even when looking closely) I tried to check the effect of this PR on #12443 , but the issue seems to be Linux-specific to begin with (I tried it on FireFox on Windows, and couldn't see these inflated labels here. Maybe @anne-gropler wants to try out this branch with the sandcastle from that PR?) I re-ran the Sandcastle from #11747 (with minor adjustments) - I'll post it as code here, the relevant points are summarized below, var viewer = new Cesium.Viewer("cesiumContainer");
viewer._cesiumWidget._creditContainer.style.display = "none";
const e = viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(0, 0, 100),
billboard: {
image: Cesium.writeTextToCanvas(
" top-- ",
{ backgroundColor: Cesium.Color.RED }
),
scale: 2,
},
});
viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(0, -0.00003, 100),
billboard: {
image: Cesium.writeTextToCanvas("--'----", {
backgroundColor: Cesium.Color.RED,
}),
scale: 2,
},
});
viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(0, -0.00006, 100),
billboard: {
image: Cesium.writeTextToCanvas("- ___- - - - '", {
backgroundColor: Cesium.Color.RED,
}),
scale: 2,
},
});
viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(0, -0.00009, 100),
billboard: {
image: Cesium.writeTextToCanvas("__--__--__--", {
backgroundColor: Cesium.Color.RED,
}),
scale: 2,
},
});
// This crashes on `main` with
// Error loading image for billboard: DeveloperError: Expected image.height to be greater than or equal to 1, actual value was 0
// but works now due to the
// height: Math.max(Math.round(height), 1),
/*/
viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(0, -0.00012, 100),
billboard: {
image: Cesium.writeTextToCanvas("_____", {
backgroundColor: Cesium.Color.RED,
}),
scale: 2,
},
});
//*/
// This crashes on `main` (see above)
/*/
viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(0, -0.00015, 100),
billboard: {
image: Cesium.writeTextToCanvas("------", {
backgroundColor: Cesium.Color.RED,
}),
scale: 2,
},
});
//*/
viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(0, -0.00018, 100),
billboard: {
image: Cesium.writeTextToCanvas("Bottom Text", {
backgroundColor: Cesium.Color.RED,
}),
scale: 2,
},
});
viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(0, -0.00021, 100),
billboard: {
image: Cesium.writeTextToCanvas("Right-to-left: Master (אדון): Hello תלמיד (student): שלום", {
backgroundColor: Cesium.Color.RED,
}),
scale: 2,
},
});
viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(0, -0.00024, 100),
billboard: {
image: Cesium.writeTextToCanvas("Below this is a space only", {
backgroundColor: Cesium.Color.RED,
}),
scale: 2,
},
});
// This (a single space) crashes on `main`
// AND on this branch. Consider changing
// isSpace: ... height: 0
// to
// isSpace: ... height: 1
// to avoid that
/*/
viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(0, -0.00027, 100),
billboard: {
image: Cesium.writeTextToCanvas(" ", {
backgroundColor: Cesium.Color.RED,
}),
scale: 2,
},
});
//*/
viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(0, -0.00030, 100),
billboard: {
image: Cesium.writeTextToCanvas("Above this is a space only", {
backgroundColor: Cesium.Color.RED,
}),
scale: 2,
},
});
viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(0, -0.00034, 100),
billboard: {
image: Cesium.writeTextToCanvas("꧁Javanese rerenggans꧂", {
backgroundColor: Cesium.Color.RED,
}),
scale: 2,
},
});
viewer.entities.add({
position: Cesium.Cartesian3.fromDegrees(0, -0.00038, 100),
billboard: {
image: Cesium.writeTextToCanvas(" À̴̴̴̴̴̖̖̖̖̖̀̀̀̀ n example for unicode combined characters", {
backgroundColor: Cesium.Color.RED,
}),
scale: 2,
},
});
viewer.flyTo(viewer.entities, {
duration: 0
});Relevant points:The cases of
Both have ~"height of 0", apparently. They are working with this PR, due to that The case of if (isSpace) {
return {
width: metrics.width,
height: 1, // ------------------------ ... return 1 here...
ascent: 0,
descent: 0,
minx: 0,
};
}... could prevent that crash. One could argue about what should be rendered there. (Nothing? A line? A rectangle?...). But I think that this is something that should at least be anticipated. In doubt, this could be some user input, and the application should not have to manually check this case to prevent a crash. Using And... the ꧁Javanese rerenggans꧂are still working. Again: I'll do the "final" review a bit later. I already looked at the code, and there doesn't seem to be anything controversial. The main "issue" fow now is about that temporary/local |
|
Thanks @javagl!
Interesting! I'll re-run on my machine just in case I've accidentally changed something, but it may indeed be a browser/OS/hardware difference. I'd originally run the tests in Chrome, on macOS, connected to an external display with resolution such that I'll try some other browsers and display settings to see what happens. If failures persist, perhaps we'll want to add a few more tests to confirm these are differences of "a pixel or two" and not something that will add up to larger problems on multi-character labels. |
|
It's not perfectly clear how thoroughly things should be tested, and how deeply ~"unexpected behaviors" should be investigated. Most of the following is just a deep dive into the craziness of font rendering. Logging my I logged some of the outputs and checked the values, to see why the tests are failing. And I found this: Sooo... the difference between the This was the case for For the Chinese characters, all tested ones pass, except for So I commented out all the where the descent and the height are failing. Easy to fix: However, a comparsion of where the better one (which doesn't cut off the bottom row) is from this PR, so I'd consider this as a plus. So there isn't any reason to not approve this, except for that Do you think that ensuring a |
This does not only apply to "multi-character labels", but also to the "small" test cases. The tests until now had been restricted to it("all of the above", () => {
const families = ["sans-serif", "serif", "monospace"];
const sizes = [6, 9, 12, 16, 24, 96];
console.log(
`family;size;char;m1.width;m1.height;m1.ascent;m1.descent;m1.minx;m2.width;m2.height;m2.ascent;m2.descent;m2.minx;absDiff(width);absDiff(height);absDiff(ascent);absDiff(descent);absDiff(minx)`,
);
for (const f of families) {
for (const s of sizes) {
ctx.font = `${s}px ${f}`;
for (const char of "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ世界您好áéíóúüñ¿¡çéâêîôûàèìòùëïüß".split(
"",
)) {
const m1 = measureText1(ctx, char, ctx.font, false, true, 1);
const m2 = measureText2(ctx, char, ctx.font, false, true, 2);
const d_width = Math.abs(m1.width - m2.width);
const d_height = Math.abs(m1.height - m2.height);
const d_ascent = Math.abs(m1.ascent - m2.ascent);
const d_descent = Math.abs(m1.descent - m2.descent);
const d_minx = Math.abs(m1.minx - m2.minx);
console.log(
`${f};${s}px;${char};${m1.width};${m1.height};${m1.ascent};${m1.descent};${m1.minx};${m2.width};${m2.height};${m2.ascent};${m2.descent};${m2.minx};${d_width};${d_height};${d_ascent};${d_descent};${d_minx}`,
);
//expect(Math.abs(m1.width - m2.width)).toBeLessThanOrEqual(MAX_ERR_PX);
//expect(Math.abs(m1.height - m2.height)).toBeLessThanOrEqual(MAX_ERR_PX);
//expect(Math.abs(m1.ascent - m2.ascent)).toBeLessThanOrEqual(MAX_ERR_PX);
//expect(Math.abs(m1.descent - m2.descent)).toBeLessThanOrEqual(MAX_ERR_PX);
//expect(Math.abs(m1.minx - m2.minx)).toBeLessThanOrEqual(MAX_ERR_PX);
}
}
}
});Yeah, it's not really a "test case". It just dumps some data to the console. This data is attached here: The differences increase with the font size. That may be expected. For example, for does not show a significant difference. I have not yet investigated why that is - maybe none of the callers of that Another "dimension of things that may have to be tested" is the handling of But... the room for tests here is nearly infinite. For me, the |
Does this link crash on |
|
While it does contribute to passing the proposed unit test, I'm worried that The custom implementation was unable to detect minx values outside the (hard-coded) padding, and perhaps wasn't quite measuring the same thing, I think. So there's a subtle change here. I am tempted to remove the
|
It's hard to imagine how it could break the layout, but not knowing all details of that glyph-based atlassing, it's hard to be sure. (I just saw that there are (surprisingly many) whitespace characters, and one could go the extra mile an try to run ~"tests with all of them", but... I think that it would still be fair to categorize them into 'space' and 'all the others'...)
Yes! Both Chrome and Firefox (on Windows (with a GeForce 2080 (with a device pixel ratio of 1 (on a Tuesday)))) are crashing with This is somehow not unexpected, with the remaining mystery of where a non-0-value is coming from on your system. A wild guess: These lines/characters have a ~"height of 0.8", and with some device pixel ratio based scaling of 2.0 this becomes
I'd have to re-read the details, but from a quick look, there's some padding of 100, which should be enough...? |


Description
Revival of @javagl's #11747. One difference from the original PR, I've aimed to replace the custom measureText implementation with as little divergence from the current results as possible. So I've intentionally not tried to fix #10649 here. Along the same lines, I'm rounding some metrics to the nearest pixel to match the previous behavior. This helps with the test case shown below.
Issue number and link
writeTextToCanvasandmeasureTextfunction #9767Testing plan
Primarily, I've tested the PR by programmatically comparing output metrics with the previous implementation, for sample characters in English, Spanish, French, and Chinese Simplified alphabets:
The purpose of the test is to confirm that no metric is off by more than 1px, for any character, compared to the previous implementation. The tests pass. Notably, if the
Math.roundcalls are removed from the implementation, many of these tests begin to fail.I also did some basic screenshot comparisons. Visual results are not identical, but — per the 1px target — are very close.
Labels
Difference of a few pixels. Main concern here would be if the font baseline is misaligned, but both appear fine in tests so far.
Sandbox from #11747
Some visible differences. Unscientifically, I think
writeTextToCanvasis now more tightly fitting the canvas to the text when padding=0, which seems ... not bad? Based on #10649, the previous behavior might not be ideal, but I've tried to just minimize changes and avoid a regression in this PR so far, leaving a fix for future work.Sandbox from #11705
Improvement. The padding in the original screenshot was a bug, believed to be caused by our custom implementation not supporting some fonts correctly.
Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my changePR Dependency Tree
This tree was auto-generated by Charcoal