Improve label readability #188

mramato · 2012-08-28T20:58:16Z

A while back I tweaked BillboardCollectionVS.glsl to align to pixel in one direction. This improved text legibility somewhat, but in many cases text still gets pretty ugly. It also caused images to look worse in some cases. This change adds a BillboardCollection.clampToPixel boolean property which aligns all vertices to a pixel in screen space. I then updated the LabelCollection to turn it on by default (it's off by default for Billboards).

The end result is more readable text and smoother non-text billboard animation.

Look out @pjcozzi I'm learning just enough about graphics to be dangerous ;)

A while back I tweaked BillboardCollectionVS.glsl to align to pixel in one direction. This improved text legibility somewhat, but in many cases text still gets pretty ugly. It also caused images to look worse in some cases. This change adds a `BillboardCollection.clampToPixel` boolean property which aligns all vertices to a pixel in screen space. I then updated the LabelCollection to turn it on by default (it's off by default for Billboards). The end result is more readable text and smoother non-text billboard animation.

Fix breakage from my last commit. Expose `clampToPixel` as an option on `LabelCollection`.

Conflicts: CHANGES.md

pjcozzi · 2012-08-28T22:03:59Z

Source/Scene/BillboardCollection.js

@@ -163,6 +163,15 @@ define([
        this._projection = undefined;

        /**
+         * If true, aligns all vertices to a pixel in screen space,


I'm not sure that we want to say "vertices" here. It is precise, but maybe too precise for some users. Perhaps just "billboards?"

Yeah, I wasn't too keen on my description either, I'll update it but couldn't think of anything better. I'll update it.

pjcozzi · 2012-08-28T22:51:46Z

Source/Scene/BillboardCollection.js

@@ -658,7 +667,11 @@ define([
            blending : BlendingState.ALPHA_BLEND
        });

-        this._sp = context.getShaderCache().getShaderProgram(BillboardCollectionVS, BillboardCollectionFS, attributeIndices);
+        this._sp = context.getShaderCache().getShaderProgram(


There is a pretty significant bug here, and below when assigning to _spPick. Do you see it?

.
.
.

When does this get called? What happens if clampToPixel changes? Does that trigger a recompile to change the #define?

However, I don't think we need the #define; a uniform should be OK. The shader could do something like

mix(vec2(positionWC.x, positionWC.y), vec2(floor(positionWC.x), floor(positionWC.y)), u_clampToPixel);

where u_clampToPixel is a floating-point uniform that is 0.0 for false or 1.0 for true. The GLSL compiler should do a good job of optimizing this. Even if not, the floor calls probably aren't too bad.

I'll update the code to use a uniform instead of a #define.

As far as the bug goes, I didn't notice the tricky this.updateForPick = function(context) {}; after the compile. I understand it's an optimization, but a comment at the beginning of the function mentioning it would be nice.

I'm actually shying away from code like this.updateForPick = function(context) {}; now. This code is from my earlier JavaScript experimenting days.

As you can see, one PIA in our graphics code is some changes to a primitive (material changes are probably the best example) require a shader recompile, and we need to carefully track these changes, and only recompile when need be. Another strategy that I want to explore is compiling shaders on load - at least the ones we know we will use. This will also help us help the driver compile them asynchronously.

Okay, I'm still learning glsl, so maybe I'm being overzealous, but couldn't I avoid the vec2 construction and simplify your suggestion like this?

vec2 clampedXY = mix(positionWC.xy, floor(positionWC.xy), u_clampToPixel); gl_Position = czm_viewportOrthographic * vec4(clampedXY, -positionWC.z, 1.0);

Seems to work great, but I wasn't sure if there was a reason you wrote the original version like you did.

I prefer your solution because it is concise. From a performance point of view, I expect them to be identical if the compiler does any optimizations at all.

Conflicts: CHANGES.md

mramato · 2012-08-29T02:15:17Z

Okay, this should be ready.

pjcozzi · 2012-08-29T11:13:22Z

Code looks good. By the way, we can also use a vector swizzle as an l-value:

positionWC.xy = mix(positionWC.xy, floor(positionWC.xy), u_clampToPixel);

There are, however, six test failures in the CompositePrimitive. We can fix the tests by changing the character rendered (it is an 'x'), setting clampToPixel to false, or rendering a billboard instead. Up to you.

mramato · 2012-08-29T14:06:30Z

Something is screwy. I actually already changed the CompositePrimitive to turn off clampToPixel and all of the tests pass in Canary; but they fail everywhere else. I'm trying to debug it, but I can't figure out why it doesn't work.

mramato · 2012-08-29T14:28:31Z

As we saw, the test failures were more a problem with the test than the change. Everything passes now on both Chrome and Firefox.

pjcozzi · 2012-08-29T14:32:08Z

Looks good. Tests passed. Merged.

shunter · 2012-08-29T14:32:08Z

CHANGES.md

@@ -42,6 +42,7 @@ Beta Releases
 * Added `affectedByLighting` to `ComplexConicSensorVolume`, `CustomSensorVolume`, and `RectangularPyramidSensorVolume` to turn lighting on/off for these objects.
 * CZML `Polygon`, `Cone`, and `Pyramid` objects are no longer affected by lighting.
 * Added `czm_viewRotation` and `czm_viewInverseRotation` automatic GLSL uniforms.
+* Added a `clampToPixel` property to `BillboardCollection` and `LabelCollection`.  When true, it aligns all billboards and text to a pixel in screen space, providing a crisper images at the cost of jumpier motion.


"providing a crisper images" should either be "providing crisper images" or "providing a crisper image"

@mramato it's OK to make this tweak in master since I just merged this.

Just to close the loop, this has been fixed in master.

Improve label readability

Rename sensors

mramato added 5 commits August 28, 2012 16:57

I forgot to run the tests..

0f1549e

Fix breakage from my last commit. Expose `clampToPixel` as an option on `LabelCollection`.

Merge branch 'master' into snapToPixel

9b25a20

Conflicts: CHANGES.md

Merge branch 'master' into snapToPixel

d9bb14d

Merge branch 'master' into snapToPixel

628edcc

pjcozzi reviewed Aug 28, 2012
View reviewed changes

Tweak comments.

68fb7f0

pjcozzi reviewed Aug 28, 2012
View reviewed changes

mramato added 4 commits August 28, 2012 19:48

Merge branch 'master' into snapToPixel

b0eee1c

Use a uniform instead of a #define.

e649c89

Merge branch 'master' into snapToPixel

157cb51

Conflicts: CHANGES.md

Add a comment.

132a3dd

mramato added 2 commits August 29, 2012 10:07

Clean up some code, tests pass in Canary but fail everywhere else.

52a5f54

Change test to be more reliable.

4a218fc

shunter reviewed Aug 29, 2012
View reviewed changes

pjcozzi added a commit that referenced this pull request Aug 29, 2012

Merge pull request #188 from AnalyticalGraphicsInc/snapToPixel

0857150

Improve label readability

pjcozzi merged commit 0857150 into master Aug 29, 2012

shunter added a commit that referenced this pull request Oct 22, 2015

Merge pull request #188 from AnalyticalGraphicsInc/sensor-visualizers

3326aa1

Rename sensors

shunter mentioned this pull request Aug 25, 2016

Poor billboard quality #4235

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve label readability #188

Improve label readability #188

mramato commented Aug 28, 2012

pjcozzi Aug 28, 2012

mramato Aug 28, 2012

pjcozzi Aug 28, 2012

mramato Aug 28, 2012

pjcozzi Aug 28, 2012

mramato Aug 28, 2012

pjcozzi Aug 29, 2012

mramato commented Aug 29, 2012

pjcozzi commented Aug 29, 2012

mramato commented Aug 29, 2012

mramato commented Aug 29, 2012

pjcozzi commented Aug 29, 2012

shunter Aug 29, 2012

pjcozzi Aug 29, 2012

mramato Aug 29, 2012

Improve label readability #188

Improve label readability #188

Conversation

mramato commented Aug 28, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato commented Aug 29, 2012

pjcozzi commented Aug 29, 2012

mramato commented Aug 29, 2012

mramato commented Aug 29, 2012

pjcozzi commented Aug 29, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment