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

Label backgrounds #4715

Merged
merged 40 commits into from
Dec 8, 2016
Merged

Label backgrounds #4715

merged 40 commits into from
Dec 8, 2016

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Dec 5, 2016

This PR adds three options to Label primitives and LabelGraphics for entities & CZML.

  • showBackground (default: false)
  • backgroundColor (default: new Cesium.Color(0.165, 0.165, 0.165, 0.8), translucent dark gray)
  • backgroundPadding (default: new Cesium.Cartesian2(2, 2) but this might be a bit small)

Additionally, this PR adds a new value to VerticalOrigin called BASELINE, which for Billboards is just a synonym for BOTTOM, but for labels will align the text along the baseline (for example with lowercase y hanging down below the baseline). (Note that master's behavior is somewhat unpredictable now, depending on heights and descenders of glyphs, master's BOTTOM sometimes acts as a baseline and sometimes as a true bottom. For example try label oy vs Oy in master and look at the bottom alignment).

I'm getting this open for review now, to get on the agenda for the code sprint, but it's not quite ready to merge yet. The main blocker is there appears to be a crash bug when terrain, HeightReference, and showBackground are used together and then removed from the viewer. For an example of this, load the Ground Clamping sandcastle demo on this branch, and use the dropdown to switch to any other clamped entity choice. The point + label demo works, but trashes the frustum list when the entity is removed. This is tough to track down and any assistance would be appreciated.

TODO

  • Fix bug revealed by Ground Clamping demo. -- was caused by rendering empty text label with background turned on.
  • Test behavior of backgrounds when mixed with clustering. -- fixed typo that affected this.
  • Post demo I wrote for visually testing getScreenSpacePosition to justify changes there.
  • Add backgrounds and BASELINE to various label/billboard demos.
  • CHANGES.md

Separately:

Conflicts:
	Source/Scene/LabelCollection.js
Somehow this has exposed a serious bug with removing a label that
has both a label background and a height reference.  Further
investigation is needed.
@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 5, 2016

On Mac/Chrome (didn't test anything else), the background for the label in the Ground Clamping Sandcastle example jumps horizontally when the camera moves.

ok

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 5, 2016

Can you add a few examples to the Label Sandcastle example or add this to the above tasklist?

@emackey
Copy link
Contributor Author

emackey commented Dec 7, 2016

For reference, here's the code I'm using to visually inspect the results of private function getScreenSpaceBoundingBox. I was tempted to make this function public, unless there's a reason not to.

This code draws a pink DOM border around the reported position.

var viewer = new Cesium.Viewer('cesiumContainer');
//viewer.cesiumWidget.resolutionScale = 0.25;

var scene = viewer.scene;
var position = Cesium.Cartesian3.fromDegrees(-75.1641667, 39.9522222);

var pointPrimitives = scene.primitives.add(new Cesium.PointPrimitiveCollection({scene: scene}));
var pointPrimitie1 = pointPrimitives.add({
    pixelSize : 5,
    color : Cesium.Color.YELLOW,
    position : position
});

var labels = viewer.scene.primitives.add(new Cesium.LabelCollection({scene: scene}));
var label1 = labels.add({
    position : position,
    text : 'Philly',
    font : '24px Helvetica',
    fillColor : new Cesium.Color(0.6, 0.9, 1.0),
    style : Cesium.LabelStyle.FILL,
    horizontalOrigin : Cesium.HorizontalOrigin.LEFT,
    verticalOrigin : Cesium.VerticalOrigin.TOP,
    //pixelOffset : new Cesium.Cartesian2(100, 50),
    scale : 1.5,
    showBackground : true
});


var div = document.createElement('div');
div.style.border = '1px solid #f0f';
div.style.position = 'absolute';
document.body.appendChild(div);

var scratch1 = new Cesium.Cartesian3();
var rect = new Cesium.BoundingRectangle();
viewer.clock.onTick.addEventListener(function() {
    var screenSpacePosition = label1.computeScreenSpacePosition(viewer.scene, scratch1);
    if (!Cesium.defined(screenSpacePosition)) {
        div.style.top = '-100px';
        div.style.height = '0';
    } else {
        Cesium.Label.getScreenSpaceBoundingBox(label1, screenSpacePosition, rect);
        div.style.left = rect.x - 1 + 'px';
        div.style.top = rect.y - 1 + 'px';
        div.style.width = rect.width + 'px';
        div.style.height = rect.height + 'px';
    }
});

viewer.scene.fxaa = false;

Sandcastle.addToolbarMenu([{
    text : 'HorizontalOrigin.LEFT',
    onselect : function() {
        label1.horizontalOrigin = Cesium.HorizontalOrigin.LEFT;
    }
}, {
    text : 'HorizontalOrigin.CENTER',
    onselect : function() {
        label1.horizontalOrigin = Cesium.HorizontalOrigin.CENTER;
    }
}, {
    text : 'HorizontalOrigin.RIGHT',
    onselect : function() {
        label1.horizontalOrigin = Cesium.HorizontalOrigin.RIGHT;
    }
}]);

Sandcastle.addToolbarMenu([{
    text : 'VerticalOrigin.TOP',
    onselect : function() {
        label1.verticalOrigin = Cesium.VerticalOrigin.TOP;
    }
}, {
    text : 'VerticalOrigin.CENTER',
    onselect : function() {
        label1.verticalOrigin = Cesium.VerticalOrigin.CENTER;
    }
}, {
    text : 'VerticalOrigin.BASELINE',
    onselect : function() {
        label1.verticalOrigin = Cesium.VerticalOrigin.BASELINE;
    }
}, {
    text : 'VerticalOrigin.BOTTOM',
    onselect : function() {
        label1.verticalOrigin = Cesium.VerticalOrigin.BOTTOM;
    }
}]);

Sandcastle.addToolbarMenu([{
    text : 'Show background',
    onselect : function() {
        label1.showBackground = true;
    }
}, {
    text : 'Hide background',
    onselect : function() {
        label1.showBackground = false;
    }
}]);


window.Cesium = Cesium;
window.viewer = viewer;
window.myDiv = div;

@emackey
Copy link
Contributor Author

emackey commented Dec 7, 2016

This is almost ready. I still need to investigate @pjcozzi's feedback on a better way to unit-test the getScreenSpacePosition function. Also I plan to make a separate branch where I incorporate the multi-line logic from #4306, but I won't get to that today.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 7, 2016

Will review again when this is fully ready unless there is something specific you want looked at now.

@emackey
Copy link
Contributor Author

emackey commented Dec 8, 2016

It looks like the 1-pixel "shake" mentioned at the top of this review, in the Ground Clamping demo, is something that happens subtly to all billboards and glyphs when clamping is active, possibly due to small rounding errors. The sharp edge of the background billboard can make this much more noticeable than on other billboards, drawing attention to itself. But I think this is an unrelated existing issue with clamped billboards.

@emackey
Copy link
Contributor Author

emackey commented Dec 8, 2016

I believe this is ready!

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 8, 2016

It looks like the 1-pixel "shake" mentioned at the top of this review, in the Ground Clamping demo, is something that happens subtly to all billboards and glyphs...

Can you submit a separate issue now that this is more prominent?

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 8, 2016

@emackey can you run all the tests in Chrome and Firefox?

On Mac/Chrome, I have this new test failure:

Scene/LabelCollection Label computes screen space bounding box with vertical origin center
Expected -18 to be less than -18.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 8, 2016

@emackey did you test this with the various pixel and eye offset options? Do these need more unit tests?

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 8, 2016

Just those comments. I'll be online for the next few hours to merge if this is ready.

@emackey
Copy link
Contributor Author

emackey commented Dec 8, 2016

That vertical center test got bad numbers in it with my last update. Fixed.

The unit tests don't currently test combinations of features, but I did test all the combinations with backgrounds I could think of with hand-edits to the above pasted Sandcastle demo. Generally these "just work" because the background itself is a billboard just like each glyph, and the various label offsets and options are merely copied down into the supporting billboards. The clustering options were tested later in the review and revealed a typo there, now fixed. All of the other options appear to be working correctly.

This is ready.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 8, 2016

Awesome work, @emackey!

@pjcozzi pjcozzi merged commit 43c6ff4 into master Dec 8, 2016
@pjcozzi pjcozzi deleted the label-backgrounds branch December 8, 2016 19:51
@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 8, 2016

@emackey if any, please close any related issues or reply to any related forum threads.

@@ -35,6 +35,9 @@ define([
* @param {Property} [options.outlineColor=Color.BLACK] A Property specifying the outline {@link Color}.
* @param {Property} [options.outlineWidth=1.0] A numeric Property specifying the outline width.
* @param {Property} [options.show=true] A boolean Property specifying the visibility of the label.
* @param {Property} [options.showBackground=true] A boolean Property specifying the visibility of the background behind the label.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong default is specified here, FYI.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants