Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Support for icon-text-fit, icon-text-fit-padding #5334

Merged
merged 4 commits into from
Jun 15, 2016
Merged

Conversation

yhahn
Copy link
Member

@yhahn yhahn commented Jun 13, 2016

Native port of mapbox/mapbox-gl-js#2720.

Backlog

  • Style spec coordination
  • Code review

@mb12
Copy link

mb12 commented Jun 13, 2016

@yhahn Can you please kindly clarify the following?

  1. Should the default value of icon-text-fit be both instead of none? This would ensure that the shield symbolizer icon is scaled by default to cover the text on it.
  2. Also is it possible to explain what is the following number 24.0F? I've noticed this several times in the symbol placement code but I do not understand what it is for etc..

auto size = layout.textSize / 24.0f;

@yhahn
Copy link
Member Author

yhahn commented Jun 13, 2016

Should the default value of icon-text-fit be both instead of none? This would ensure that the shield symbolizer icon is scaled by default to cover the text on it.

No since this would alter the behavior of all existing icons in styles. This is an opt-in mode so it defaults to none and then if a style specifies one of the both/width/height values it enables icon-sizing/positioning based on text.

Also is it possible to explain what is the following number 24.0F? I've noticed this several times in the symbol placement code but I do not understand what it is for etc.

Yes, this is the current fixed size of the glyph SDF bitmaps used by Mapbox GL and is a "magic number" that glyph metrics are currently related to. You can think of text shaping as occurring on a 24.0-based scale independently of text size, etc.

To convert between shaped text and actual size of quads/pixels on screen the 24.0 magic number needs to be scaled by text size.

@@ -39,7 +39,21 @@ optional<std::string> parseConstant(const char* name, const JSValue& value) {

template <>
optional<Color> parseConstant(const char* name, const JSValue& value) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@jfirebaugh help : )

Defining

optional<std::array<float, 4>> parseConstant(const char* name, const JSValue& value) {

is considered a redefinition of optional<Color>. Is there a noninvasive way around this or do we just decide on the semantics here and maybe go with optional<std::array<float, 4>> as less misleading?

Other options?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should introduce a real class definition for Color, rather than using a typedef. I've been meaning to do this for a while. Want to take a stab?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, will poke you when I have questions.

@yhahn yhahn mentioned this pull request Jun 15, 2016
@yhahn yhahn force-pushed the icon-text-fit branch 3 times, most recently from 2ad1490 to 39e798b Compare June 15, 2016 15:41
@yhahn yhahn changed the title [notready] Support for icon-text-fit, icon-text-fit-padding Support for icon-text-fit, icon-text-fit-padding Jun 15, 2016
@yhahn
Copy link
Member Author

yhahn commented Jun 15, 2016

@jfirebaugh ready for review.

@jfirebaugh
Copy link
Contributor

👍

@yhahn yhahn merged commit 199ea2a into master Jun 15, 2016
@yhahn yhahn deleted the icon-text-fit branch June 15, 2016 21:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants