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

Better handling for undefined icon|text-rotation-alignment #6253

Merged
merged 2 commits into from
Sep 21, 2016

Conversation

yhahn
Copy link
Member

@yhahn yhahn commented Sep 6, 2016

I believe this addresses the root cause behind #5683.

  • Updates GL style spec to remove an explicit default from text-rotation-alignment and icon-rotation-alignment. This value was a lie given that renderers currently derive the default in coordination with symbol-placement
  • Update logic in GL native to set the default value for these properties only when they are undefined.

Remaining backlog

  • @yhahn Render tests to lockdown expected behavior
  • @yhahn Vet style spec change, esp possible effect on GL JS and other integrators
  • Review @1ec5 @jfirebaugh

@mention-bot
Copy link

@yhahn, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @mikemorris and @ansis to be potential reviewers.

} else {
bucket->layout.iconRotationAlignment.value = AlignmentType::Viewport;
}
};
Copy link
Contributor

@1ec5 1ec5 Sep 6, 2016

Choose a reason for hiding this comment

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

Nit: extra semicolons (3×).

@yhahn
Copy link
Member Author

yhahn commented Sep 6, 2016

Quick discussion with @1ec5 -- one possible approach to address the special-caseness of these properties would be to add an explicit enum value to the spec like auto or inherit for text-rotation-alignment, icon-rotation-alignment and text-pitch-alignment.

  • Add auto enum value
  • Set default value to auto
  • Update logic in GL native/JS to handle this appropriately

I'm fairly certain this would be a breaking change though -- styles explicitly using the value auto would not parse (?) with older renderers.

@jfirebaugh
Copy link
Contributor

Yes, I think we need to add auto for all three of these properties, and treat this new value like we would treat the addition of a new style property -- add a row in the spec property support table documenting the minimum versions requirements.

@boundsj boundsj added this to the ios-v3.4.0 milestone Sep 15, 2016
@boundsj boundsj self-assigned this Sep 15, 2016
@boundsj boundsj added navigation For the Mapbox Navigation SDK for Android or iOS or navigation use cases in general Core The cross-platform C++ core, aka mbgl labels Sep 15, 2016
@boundsj boundsj force-pushed the symbol-complex-default branch from 01085d3 to cdf7d39 Compare September 16, 2016 00:29
Copy link
Contributor

@boundsj boundsj left a comment

Choose a reason for hiding this comment

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

@jfirebaugh @lbud this file was falling behind so I rebased on top of master. This commit keeps @lbud's latest changes but changes s/bucket->layout/layoutProperties

@jfirebaugh
Copy link
Contributor

Changes here look good. Do we have a test case for the original issue #5683?

@boundsj
Copy link
Contributor

boundsj commented Sep 16, 2016

@jfirebaugh a PR for GL JS tests is started and is blocked by mapbox/mapbox-gl-js#3203. We still need to add render tests to this PR.

@boundsj boundsj added iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS Android Mapbox Maps SDK for Android labels Sep 19, 2016
@boundsj boundsj changed the title [core] [notready] Better handling for undefined icon|text-rotation-alignment [WIP] Better handling for undefined icon|text-rotation-alignment Sep 19, 2016
@boundsj boundsj force-pushed the symbol-complex-default branch from 6df272e to a5fbf64 Compare September 20, 2016 04:50
@boundsj boundsj changed the title [WIP] Better handling for undefined icon|text-rotation-alignment Better handling for undefined icon|text-rotation-alignment Sep 20, 2016
@jfirebaugh jfirebaugh force-pushed the symbol-complex-default branch 2 times, most recently from 4252b79 to 2325c79 Compare September 21, 2016 00:51
@jfirebaugh
Copy link
Contributor

Style-spec, test-suite, and gl-js have been updated. Remaining work here is to get the tests passing.

@jfirebaugh
Copy link
Contributor

Two of the icon-rotation-alignment tests are failing due to very slight positioning differences. 😖 I'll finish up tomorrow.

@jfirebaugh jfirebaugh force-pushed the symbol-complex-default branch from 22dd4fb to 719d278 Compare September 21, 2016 22:25
@jfirebaugh jfirebaugh force-pushed the symbol-complex-default branch from 719d278 to 24318a6 Compare September 21, 2016 22:31
@jfirebaugh jfirebaugh merged commit 6a2ce35 into master Sep 21, 2016
@jfirebaugh jfirebaugh deleted the symbol-complex-default branch September 21, 2016 23:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android Core The cross-platform C++ core, aka mbgl iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS navigation For the Mapbox Navigation SDK for Android or iOS or navigation use cases in general
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants