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

Billboards and Labels get their act together #4675

Merged
merged 17 commits into from
Nov 23, 2016
Merged

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Nov 22, 2016

I'll start by quoting the new section of CHANGES.md:

  • Billboard depth testing changed from LESS to LEQUAL, allowing label glyphs of equal depths to overlap.
  • Billboard sizes were incorrectly rounded up from odd values to even values. This has been corrected, and as a result, odd-width and odd-height billboards will appear one pixel smaller than before.
  • Label glyph positions have been adjusted and corrected.
  • TextureAtlas.borderWidthInPixels has always been applied to the upper and right edges of each internal texture, but is now also applied to the bottom and left edges of the entire TextureAtlas, guaranteeing borders on all sides regardless of position within the atlas.

I'll dig into each of these a bit.

First, @mramato's long-standing font-keming (font kerning, I see what you did there) branch has been merged in here, enabled by the depth function change from LESS to LEQUAL such that overlapping glyphs at the same depth can each get a turn with the relevant fragments. This applies not just to glyphs, but all billboards and point primitives as well, such that the translucent fringes of all those things get a fair shot at the same depth without overwriting one another. Note that this doesn't completely alleviate the need for something like billboard-translucency and its 2-pass rendering at some point in the future, but it does make this pull request completely independent from such a need, so it can be merged with single-pass label/billboard rendering.

Next, I unearthed something terrible. The billboard vertex shader needs the half-size of the billboard, for the distance from the center point to one corner. This half-size was being passed in as a compressed vertex attribute, specifically as an integer. Cutting an integer size in half, and then coercing it back to an integer again, loses the least significant bit off the value. As a result, all odd-sized billboards were actually being rendered one pixel larger, with even sizes! This resulted in labels that couldn't maintain a consistent baseline, with individual glyphs wiggling up and down by a full pixel. The fix is simple though, pass in the original full size as an integer via the compressed attribute, and cut the size in half inside the vertex shader after it's been uncompressed to a float.

In addition to all that, TextureAtlas had an issue where its borderWidthInPixels was not being applied to the bottom and left sides of the whole atlas. It was already being applied to the top and right sides of each individual texture within the atlas. The result was that some glyphs "clamped" their texture coordinates to the bottom and/or left edges, while other glyphs mixed to transparent along those same edges, depending on their position within the atlas being on one of the affected borders. This also had the effect of disrupting the look of a consistent font baseline across a label. This too has been corrected.

Here's a fun science experiment that you kids can try at home.

var viewer = new Cesium.Viewer('cesiumContainer');

viewer.entities.add({
    position : Cesium.Cartesian3.fromDegrees(-75.1641667, 39.9522222),
    point: {
        pixelSize: 18,
        color: Cesium.Color.YELLOW
    },
    label : {
        font : '12pt sans-serif',
        text : 'Philadelphia',
        scale: 5,
        horizontalOrigin : Cesium.HorizontalOrigin.CENTER,
        verticalOrigin : Cesium.VerticalOrigin.TOP,
    }
});

In master:

philly_master

This branch:

philly_branch

mramato and others added 16 commits March 4, 2015 11:57
`writeTextToCanvas` now always draws individual letters more reliably and
keeps them completely in the canvas.

`LabelCollection` now uses the `dimensions` object returned by
`writeTextToCanvas` to perform accurate kerning, producing much more
legible and visually appealing text.
# Conflicts:
#	Source/Core/writeTextToCanvas.js
Compressed vertex attributes must be integers.  Billboard halfSize is
needed for computing the distance from the center of the billboard out
to one of the corners.  But the half-size can have a .5 fractional
component, from the least significant bit of the original billboard
size.  So this 0.5 must be computed after the compressed vertex
attribute has already been unpacked.
Previously, it was only applied between textures within the interior
of the atlas, not on the lower and left sides.  It was effectively
applied on the upper and right sides, since each interior texture has
its own border on the upper and right sides.
@emackey
Copy link
Contributor Author

emackey commented Nov 22, 2016

This supersedes #2549 (font kerning).

There's some chance the TextureAtlas change here will have a positive impact on #4525 (outlines around billboards on some systems), but I'm not certain.

There's also a possibility that the off-by-one error in sizing of odd billboards was contributing to #4235 (poor billboard quality), although fxaa remains the primary culprit there.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 23, 2016

In master:

philly_master

This branch:

philly_branch

No doubt this is much better. In both, are the tops of the i's cut off?

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 23, 2016

The bottom of the a's is also cut off. It is due to scale : 5. If you set the scale to 1 and up the font size, it looks as expected.

@emackey do you know if there is already an issue for this? It doesn't need to be solved in this PR.

@@ -38,7 +38,7 @@ const float SHIFT_RIGHT1 = 1.0 / 2.0;

vec4 computePositionWindowCoordinates(vec4 positionEC, vec2 imageSize, float scale, vec2 direction, vec2 origin, vec2 translate, vec2 pixelOffset, vec3 alignedAxis, bool validAlignedAxis, float rotation, bool sizeInMeters)
{
vec2 halfSize = imageSize * scale * czm_resolutionScale;
vec2 halfSize = imageSize * scale * czm_resolutionScale * 0.5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment about why this is happening here so we don't try to "optimize" it out again?

@@ -7,6 +7,10 @@ Change Log
* Breaking changes
*
* Improved terrain/imagery load ordering, especially when the terrain is already fully loaded and we add a new imagery layer. This results in a 25% reduction in load times in many cases.
* Billboard depth testing changed from `LESS` to `LEQUAL`, allowing label glyphs of equal depths to overlap.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also points and labels.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 23, 2016

There's some chance the TextureAtlas change here will have a positive impact on #4525 (outlines around billboards on some systems), but I'm not certain.

I can't confirm if this fixes #4525 as I don't have this issue in master, but I have seen it in the past perhaps on another machine.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 23, 2016

@emackey just those comments. I suggest that you make those trivial tweaks and then we merge this into master so users could get some time with it before the 1.28 release.

@emackey
Copy link
Contributor Author

emackey commented Nov 23, 2016

@pjcozzi I chose scale : 5 for extreme terribleness, but it may have been the wrong thing to show off here. I know it looks weird, but it's actually not cut off. If you check out the 12pt font at its normal resolution, you'll see the dot on the i really is that much wider than tall at that size. Hard to believe, but:

philly_actual_scale

philly_x3

In the above image, magnified 3x, the top text is an actual HTML DOM element floating above the canvas, and the bottom text is a label rendered on this branch (partially obscured by a yellow point). fxaa is off but of course the billboards aren't perfectly pixel-aligned, so it's slightly more blurry than the DOM text. But you can see the glyphs are not cut off inappropriately.

(I'll include the code here for future reference)

<style>
    @import url(../templates/bucket.css);
</style>
<div id="cesiumContainer" class="fullSize"></div>
<div id="loadingOverlay"><h1>Loading...</h1></div>
<div id="toolbar"></div>
<div style="font: 12pt sans-serif; position: absolute; 
            top: 40%; left: 45%; color: white;">Philadelphia<div>
var viewer = new Cesium.Viewer('cesiumContainer');

viewer.entities.add({
    position : Cesium.Cartesian3.fromDegrees(-75.1641667, 39.9522222),
    point: {
        pixelSize: 5,
        color: Cesium.Color.YELLOW
    },
    label : {
        font : '12pt sans-serif',
        text : 'Philadelphia',
        scale: 1,
        horizontalOrigin : Cesium.HorizontalOrigin.CENTER,
        verticalOrigin : Cesium.VerticalOrigin.TOP,
    }
});

viewer.scene.fxaa = false;

@pjcozzi pjcozzi merged commit 00cee6b into master Nov 23, 2016
@pjcozzi pjcozzi deleted the billboards-dont-wiggle branch November 23, 2016 20:30
@mramato
Copy link
Contributor

mramato commented Nov 29, 2016

@emackey I know I'm late to the party (been crazy busy) but I just got a chance to look at this and wanted to say it's awesome. Thanks a lot of finishing up what was our longest running PR branch.

@gezonthenet
Copy link

gezonthenet commented Dec 6, 2016

Hi, If we want to have the label in-front of the point, how do we go about it after this? My use case was having an oversized point with label text centered in the point showing the count of objects represented by the point. After this update the label is hidden behind the point. Cheers, Gerard.
edit: for my billboard/label combo I used the eyeOffset to solve this. Looking into the point/label combo now.

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

Successfully merging this pull request may close these issues.

4 participants