Label clarity improvements #565

Merged
merged 7 commits into from Mar 13, 2013

Projects

None yet

2 participants

@mramato
Member
mramato commented Mar 12, 2013
  1. Fix issues with how we measured test so that the result is more accurate. We were using the fill-only measurement for both fill and stroke. This dramatically improves our ability to outline smaller fonts, see the update simple.czml for an example.
  2. Add the ability to specify outlineWidth for Labels and CZML labels.
  3. Default LabelCollection.clampToPixel to false, I'm not sure we even need it anymore as the fixes for 1 eliminate the reason it was added.
  4. Tweaked the Sandcastle Labels example to have a custom outlineWidth
  5. Updated changes.
mramato added some commits Mar 12, 2013
@mramato mramato First cut at improved label outline support. 7526da3
@mramato mramato More label improvements. 8708d3c
@mramato mramato Cleanup outline code
1. Partially revert Sandcastle example
2. Update CHANGES
3. Fix a failing test (by removing an uneeded check).
4. Update simple.czml to take advantage of improved outlines.
2789eb8
@mramato mramato Hopefuly finally changes before pull request 1129478
@pjcozzi
Member
pjcozzi commented Mar 13, 2013

This pull request cannot be automatically merged.

@mramato can you merge master?

@pjcozzi pjcozzi commented on the diff Mar 13, 2013
Source/Scene/Label.js
+
+ /**
+ * Sets the outline width of this label.
+ *
+ * @memberof Label
+ *
+ * @param {Number} value The outline width.
+ *
+ * @exception {DeveloperError} value is required.
+ *
+ * @see Label#getOutlineWidth
+ * @see Label#setFillColor
+ * @see Label#setFont
+ * @see <a href='http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#fill-and-stroke-styles'>HTML canvas 2D context fill and stroke styles</a>
+ */
+ Label.prototype.setOutlineWidth = function(value) {
@pjcozzi
pjcozzi Mar 13, 2013 Member

Did we add tests for these?

@mramato
mramato Mar 13, 2013 Member

Thanks for calling BS, I was being a bit lazy on the testing side. I'm confident we cover the case in course-grain label tests, but I'll verify that and add new tests where needed.

@mramato
mramato Mar 13, 2013 Member

Specs are in. I also found a bug in LabelCollection.remove and added Specs for other Label/LabelCollection code that wasn't being covered. Both objects now have 100% coverage.

@pjcozzi
Member
pjcozzi commented Mar 13, 2013

Default LabelCollection.clampToPixel to false, I'm not sure we even need it anymore as the fixes for 1 eliminate the reason it was added.

If we're sure it's no longer needed (@shunter did you add it?), I am for removing it.

@pjcozzi
Member
pjcozzi commented Mar 13, 2013

Outline text looks way better.

Are we sure non-outlined text is right?

image

image

In the previous version (top), the text was a bit thicker and aligned slightly differently. Is that because it was wrong?

@pjcozzi
Member
pjcozzi commented Mar 13, 2013

@mrmattf or @shunter - you may want to have a quick look.

@mramato
Member
mramato commented Mar 13, 2013

If we're sure it's no longer needed (@shunter did you add it?), I am for removing it.

I'm actually the one that originally added it. The only reason I didn't remove it was because I wanted everyone to agree it wasn't actually needed any longer.

@mramato
Member
mramato commented Mar 13, 2013

In the previous version (top), the text was a bit thicker and aligned slightly differently. Is that because it was wrong?

I'm fairly certain that this is an artifact of turning clampToPixel off. If you go into LabelCollection and turn it back on (or just do that in the Sandcastle example) the line becomes thicker again (unless I'm seeing what I want to see). Ultimately I'm fine with this, since smaller labels look all-around better with it turned off. (and smaller labels are what we normally have).

@mramato mramato Specs for outlineWidth
Also fleshed out other missing LabelCollection specs to bring coverage up to 100%.  Fixed a bug in `LabelCollection.remove`
4006303
@mramato
Member
mramato commented Mar 13, 2013

So the only thing left is to decide whether we should get rid of clampToPixel or not. It's easy to put back later if we have a good reason for it..

@mramato
Member
mramato commented Mar 13, 2013

After offline discussion, we've decided to remove clampToPixel for now. We can always put it back if there's a reason.

@mramato
Member
mramato commented Mar 13, 2013

This is ready for another review.

@pjcozzi pjcozzi merged commit 8a02251 into master Mar 13, 2013
@pjcozzi pjcozzi deleted the labelOutlineWidth branch Mar 13, 2013
@pjcozzi
Member
pjcozzi commented Mar 13, 2013

All good. Merged.

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