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

Implement 'circle-pitch-alignment' property #9426

Merged
merged 2 commits into from
Jul 6, 2017

Conversation

ChrisLoer
Copy link
Contributor

Port from GL JS PR mapbox/mapbox-gl-js#4871.

Fixes issue #9349.

This is my first time using the generate-style-code script. Everything that it spit out looks reasonable, but I'm not sure what extra steps I should take to test the Android/iOS code it generates. I'm not even sure what the auto-generated transition code does.

The one concern I had was that it wanted to interpret anything ending in -alignment as an AlignmentType, which allows an auto value. We don't have an auto value for circle-pitch-alignment, so maybe we should override that with a stricter type? On the other hand, if someone did use auto, the effect would end up being to use the default viewport value, so it wouldn't be a very surprising result.

/cc @jfirebaugh

@jfirebaugh
Copy link
Contributor

jfirebaugh commented Jul 6, 2017

How about we allow an auto value as discussed in mapbox/mapbox-gl-js#4871 (taking the value of circle-pitch-alignment), but leave viewport as the default?

@ChrisLoer
Copy link
Contributor Author

ChrisLoer commented Jul 6, 2017

How about we allow an auto value as discussed in mapbox/mapbox-gl-js#4871 (taking the value of circle-pitch-alignment), but leave viewport as the default?

In that discussion scaling was going to be based on alignment. Are you proposing doing it in the opposite direction, with alignment based on the scaling choice? In my mind alignment seems more fundamental than scaling, but I don't know how to justify that position.

@jfirebaugh
Copy link
Contributor

Oh, right. I was thinking of {text,icon}-pitch-alignment, where auto matches -rotation-alignment, which we don't have for circles. So never mind, I agree with you.

@ChrisLoer ChrisLoer merged commit d5fbcb2 into master Jul 6, 2017
@ChrisLoer ChrisLoer deleted the cloer_circle_pitch_alignment branch July 6, 2017 16:39
@kkaefer kkaefer added the Core The cross-platform C++ core, aka mbgl label Jul 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants