Added support for multiline labels using explicit newline character. #4306

dwhipps · 2016-09-09T16:55:26Z

I've added support for multiline text in Labels using explicit newline characters, per this issue:

#2402

I revived and (very slightly) improved the code written by @Andre-Nunes in this pull request:

#2575

I've tested and ensured that single-line labels have the same positioning (i.e. this maintains backwards compatibility)

…cit newline characters.

hpinkos · 2016-09-09T17:08:43Z

Thanks @dwhipps! It looks like you have a jsHint error
We'll take a look at this soon

dwhipps · 2016-09-09T17:37:18Z

Hannah,

I can't see the error on my end when running 'npm release' or 'npm tests-non-webgl'

Can you point me to the file/line? The "details" link in providing the Travis CI build log isn't very helpful.

hpinkos · 2016-09-09T17:39:30Z

Source/Scene/LabelCollection.js

+            // Subtract maxGlyphHeight for backwards compatibility
+            heightOffset += (totalHeight * scale) - maxGlyphHeight;
+        }
+        else if (verticalOrigin === VerticalOrigin.TOP) {


@dwhipps this is causing the jsHint error, because this block is empty. You can just remove it.

You can run jsHint with npm run jsHint

Also, update the formatting in the block above this while you're in there. It should be

} else if () {

dwhipps · 2016-09-13T12:57:23Z

I managed to fix the error. Not sure what the "bug bash" tag means. Do I have anything else to do?

hpinkos · 2016-09-13T13:36:59Z

Thanks @dwhipps, we're a little busy around here this week so I'm not sure if we'll have a chance to review this right away. I looked through and the code looks fine to me, but one of the graphics guys with a better understanding of the changes here will do a more thorough review to get this merged.

mramato · 2016-09-13T13:41:45Z

I know this code pretty well and this is a long overdue, mostly straightforward, change. I'll try and get it merged by the end of the week.

As for the bug bash label, we are having a 3-day code sprint in October to help address a lot of issues that have been reported in Cesium while most of us have been heads down on 3D Tiles related work. So the label just means that we definitely want to look at this for the sprint if it's not already merged by then.

dwhipps · 2016-09-15T14:44:14Z

I've discovered problems with this and have not yet been able to resolve them. It would recommend NOT pulling this until I've figured out what's going on.

mramato · 2016-09-15T14:54:16Z

Thanks for the heads up @dwhipps I had a few edge cases I wanted to be sure to check myself as well, so glad you're on the ball! Getting good font measurements in the browser is way harder than it should be (especially getting the same results cross-browser)

dwhipps · 2016-09-21T19:29:14Z

Red Herring. The new multiline labels code was not causing the issues I'm seeing (I'll report those separately.) As far as I can tell, it works pretty well.

kring · 2016-10-08T10:57:27Z

@mramato I'm a little late to the party here, but I'm curious why measuring text has to be so hard. Can't we just ask the browser to do it? Here's how I did it recently in a similar situation:
https://github.com/TerriaJS/terriajs/blob/master/lib/Map/Legend.js#L188

In short: Create a span, set its styles and textContent, add it to the DOM (position: fixed so it doesn't affect layout, and opacity: 0 so it's not visible) and then call getBoundingClientRect.

One thing to keep in mind is that the measurement will be affected by the inherited CSS wherever you add the element, but you can deal with that pretty easily just by adding a class to your measure element that resets everything to a known state.

Worked well for my use-case (word wrapping in an SVG), anyway.

hpinkos · 2016-10-17T18:06:05Z

@dwhipps is this ready to be reviewed or were there still some things you wanted to change?

dwhipps · 2016-10-17T18:06:58Z

Yeah, this is ready to go.

mramato · 2016-10-17T19:55:30Z

In short: Create a span, set its styles and textContent, add it to the DOM (position: fixed so it doesn't affect layout, and opacity: 0 so it's not visible) and then call getBoundingClientRect.

It's been a while and I would need to look into it, but we also need to account for letter placement, i.e. letters that go below the baseline, like lowercase y. It's not just simple width/height we need to worry about.

kring · 2016-10-19T09:24:28Z

we also need to account for letter placement, i.e. letters that go below the baseline, like lowercase y.

I don't think this should be an issue. The size of the span will account for any possible drop below the baseline. You can easily prove this to yourself with jsfiddle:
https://jsfiddle.net/srmvjdnn/1/

Change that o to a y or an R and notice that the height of the span doesn't change.

If you need to know the exact baseline position within that span for some reason, I don't know how to do that offhand, though I bet it can be done. But I can't really think of why you'd need to know that.

mramato · 2016-10-19T14:01:21Z

If you need to know the exact baseline position within that span for some reason, I don't know how to do that offhand, though I bet it can be done. But I can't really think of why you'd need to know that.

I think the main issue with your approach is the wasted space above the y. I'm less concerned about the memory/texture usage and more concerned with how that affects origin/offset positioning (though I admit it may not matter). @shunter was actually the original author for most of the current label code, so hopefully he can chime in with some thoughts. I'm fine with trying to approach you are suggesting, especially if it simplifies the code.

mramato · 2016-10-19T15:04:01Z

@dwhipps Check out the below code snippet and screenshot, I would expect the point to be dead center. It is in master (but without the new lines obviously). We need to make sure all of the various origin combinations work as expected.

var viewer = new Cesium.Viewer('cesiumContainer');
viewer.entities.add({
    position : Cesium.Cartesian3.fromDegrees(-75.1641667, 39.9522222),
    point:{
        color: Cesium.Color.BLUE,
        pixelSize:15,
        verticalOrigin : Cesium.VerticalOrigin.CENTER,
        horizontalOrigin : Cesium.HorizontalOrigin.CENTER,
    },
    label : {
        text : 'aa aa\naa aa\naa aa\naa aa',
        verticalOrigin : Cesium.VerticalOrigin.CENTER,
        horizontalOrigin : Cesium.HorizontalOrigin.CENTER,
    }
});

Keeping up to date with master

dwhipps · 2016-10-19T18:26:06Z

Hmm. Well, now I've merge in master (just to keep things up to date) and the "Point" has disappeared. Can you confirm?

Starying up to date... again.

dwhipps · 2016-10-19T19:18:24Z

Looks like the "Point" is not drawing on the latest Chrome or Safari on Sierra, but it is working on FireFox...

hpinkos · 2016-10-19T19:32:56Z

I just checked out your branch @dwhipps and the point it still drawing for me in Chrome

…lines.

hpinkos · 2016-12-07T18:58:29Z

@dwhipps you have a jsHint error:

Source/Scene/LabelCollection.js
line 270 col 51 'HeightReference' is not defined.
⚠ 1 warning

mramato · 2016-12-08T17:31:49Z

Scene/LabelCollection Label should increase label height and decrease width when adding newlines is still failing in all browsers for me. Looking at the code, I think the problem is that Label.getScreenSpaceBoundingBox needs to be updated to actually support newline characters. Does this test pass for you @dwhipps?

emackey · 2016-12-08T18:44:06Z

@mramato @dwhipps The Label.getScreenSpaceBoundingBox function has a number of issues that are fixed on the label-backgrounds branch currently under review. I changed the logic for that function and more generally I revamped the logic for placing glyphs, so there will be major merge conflicts if someone attempts a simple merge of the two branches.

I want to take a shot at creating a multiline-label-backgrounds branch, hopefully today or tomorrow, that uses my new glyph placement logic augmented with the newline processing found here, and including backgrounds for multiline labels.

dwhipps · 2016-12-08T18:46:20Z

@emackey Good Luck! Let me know if you need anything. I'm around most of the day tomorrow.

mramato · 2016-12-08T18:49:34Z

Thanks @emackey, sounds like you have it under control!

…kgrounds Conflicts: Source/Scene/LabelCollection.js

emackey · 2016-12-09T20:07:32Z

Nice, GitHub allowed me to push to your branch since this PR is open. @dwhipps take a look at what changed here, and also try setting showBackground on a label with newline chars.

One intentional change I made is that HorizontalOrigin now controls text justification as well as the position relative to the anchor point. I didn't think it made sense to have left-justified text on a label when the anchor was CENTER or RIGHT.

@mramato I think this is ready. Code I added should be reviewed. Thanks.

emackey · 2016-12-12T15:16:10Z

Updated doc & a demo. This is ready.

mramato · 2016-12-13T19:13:00Z

Sorry this took so long to get merged, but looks like the next Cesium release is going to have lots of nice label improvements. Thanks @dwhipps! and Thank @emackey for handling the merge and bounding box fixes.

pjcozzi · 2016-12-13T20:07:30Z

Source/Scene/LabelCollection.js

-        var maxY = 0;
+        var lastLineWidth = 0;
+        var maxLineWidth = 0;
+        var lineWidths = [];


Should this be a scratch?

My hope is we don't need a scratch in this case. This function only gets called when the glyphs all need to be repositioned, which is an expensive operation we try to avoid (using the delay tactics you pointed out in the label background PR).

pjcozzi · 2016-12-13T20:09:51Z

Source/Scene/LabelCollection.js

-            if (glyphIndex < glyphLength - 1) {
-                var nextGlyph = glyphs[glyphIndex + 1];
-                glyphPixelOffset.x += ((dimensions.width - dimensions.bounds.minx) + nextGlyph.dimensions.bounds.minx) * scale * resolutionScale;
+        if (defined(backgroundBillboard) && (text.split('\n').join('').length > 0)) {


Is there a more memory efficient way to do this?

text.split('\n').join('').length > 0

If so, does it matter here?

This too is part of repositionAllGlyphs and typically happens only when there is some substantial change made to the label.

pjcozzi · 2016-12-13T20:11:09Z

Tests and Sandcastle look good. Is coverage for the new code solid? Just those trivial questions.

I'd like to get this into master soon for some use before the 1.29 release. Does anyone else want to look at this?

pjcozzi · 2016-12-13T20:12:00Z

Ah, I see this was already merged; I had the window open from earlier today. 👍

arafat5549 · 2017-11-08T07:38:10Z

does it able to have richtext?
like redblue !so i can display multi color text in one line !

hpinkos · 2017-11-08T15:32:39Z

Hi @arafat5549,
No, you can only have one text color for each label. In the future, please ask questions like this on our forum: https://groups.google.com/forum/?hl=en#!forum/cesium-dev
The forum has higher traffic so it is more likely someone will see your question and be able to help. We may miss your question if you comment on a closed pull request like this. Thanks!

dwhipps added 3 commits September 9, 2016 11:06

Reanimated abandoned support for multiline text in Labels using expli…

d47f517

…cit newline characters.

Fixed to ensure backwards compatibility. Better variable naming.

bac19f0

Cleanup.

1afb9a9

hpinkos reviewed Sep 9, 2016
View reviewed changes

Removed empty block.

81a3a78

hpinkos added the bug bash label Sep 12, 2016

Merge branch 'master' into feature/label-newline-support

42c0fe9

Keeping up to date with master

Merge branch 'master' into feature/label-newline-support

c4b8703

Starying up to date... again.

dwhipps added 4 commits October 19, 2016 15:36

Looks like newline glyphs had non-zero width. Dont add widths for new…

9f8e686

…lines.

Cleanup before a bit more tweaking.

58cc9bb

Ensure we use computedWidth rather than simple width for glyph width.

7e65a65

Ensure vertical centering works correctly.

77ed7b4

Fixed undefined.

cc49261

emackey mentioned this pull request Dec 7, 2016

Label backgrounds #4715

Merged

6 tasks

emackey added 7 commits December 8, 2016 16:43

Merge remote-tracking branch 'origin/master' into multiline-label-bac…

a5d426d

…kgrounds Conflicts: Source/Scene/LabelCollection.js

CHANGES.md

1a62c75

Integrate logic for newlines and background size/placement.

84d0071

Line spacing, shield against empty string with newlines.

e17a393

Line spacing

8fbfaa3

Fix text justification to match horizontal origin choice.

40737cb

Update tests

bcc1f86

emackey added 2 commits December 9, 2016 15:20

Update doc.

72697cf

Update Sandcastle picking demo.

6539033

pjcozzi and others added 3 commits December 13, 2016 12:56

Tweak CHANGES.md

ef86d83

Mention CZML in CHANGES.md

02c6d93

Fix formatting.

fe84bef

mramato merged commit 9671dd9 into CesiumGS:master Dec 13, 2016

pjcozzi reviewed Dec 13, 2016

View reviewed changes

pjcozzi mentioned this pull request Jan 2, 2017

Support multi-line labels #2402

Closed

Added support for multiline labels using explicit newline character. #4306

Added support for multiline labels using explicit newline character. #4306

Conversation

dwhipps commented Sep 9, 2016

hpinkos commented Sep 9, 2016

dwhipps commented Sep 9, 2016

hpinkos Sep 9, 2016

Choose a reason for hiding this comment

hpinkos Sep 9, 2016

Choose a reason for hiding this comment

dwhipps commented Sep 13, 2016

hpinkos commented Sep 13, 2016

mramato commented Sep 13, 2016

dwhipps commented Sep 15, 2016

mramato commented Sep 15, 2016

dwhipps commented Sep 21, 2016

kring commented Oct 8, 2016 • edited Loading

hpinkos commented Oct 17, 2016

dwhipps commented Oct 17, 2016

mramato commented Oct 17, 2016

kring commented Oct 19, 2016

mramato commented Oct 19, 2016

mramato commented Oct 19, 2016

dwhipps commented Oct 19, 2016

dwhipps commented Oct 19, 2016

hpinkos commented Oct 19, 2016

hpinkos commented Dec 7, 2016

mramato commented Dec 8, 2016

emackey commented Dec 8, 2016

dwhipps commented Dec 8, 2016

mramato commented Dec 8, 2016

emackey commented Dec 9, 2016

emackey commented Dec 12, 2016

mramato commented Dec 13, 2016

pjcozzi Dec 13, 2016

Choose a reason for hiding this comment

emackey Dec 13, 2016

Choose a reason for hiding this comment

pjcozzi Dec 13, 2016

Choose a reason for hiding this comment

pjcozzi Dec 13, 2016

Choose a reason for hiding this comment

emackey Dec 13, 2016

Choose a reason for hiding this comment

pjcozzi commented Dec 13, 2016

pjcozzi commented Dec 13, 2016

arafat5549 commented Nov 8, 2017

hpinkos commented Nov 8, 2017

kring commented Oct 8, 2016 •

edited

Loading