Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement 'circle-pitch-alignment' property #4871

Merged
merged 2 commits into from
Jul 3, 2017

Conversation

ChrisLoer
Copy link
Contributor

Finishing up the style properties requested in #4120.

The combination of circle-pitch-alignment: map and circle-pitch-scaling: viewport is supported, but it's a bit of an odd combination: in the distance, circles will be "squashed" by the perspective effect but they'll maintain the same width. Because this is an odd choice, it might make sense to add a new (default) auto option to circle-pitch-scaling and make it key off of circle-pitch-alignment?

/cc @jfirebaugh @nickidlugash

Follows model of 'text-pitch-alignment'. Requested in issue #4120.
@nickidlugash
Copy link

I'm not that familiar with use cases for circle layers, but I'm wondering if we even need this property? Are there use cases where circle-pitch-alignment: viewport is the desired effect? Could circle-pitch-alignment: map just be the default behavior?

@ChrisLoer
Copy link
Contributor Author

@nickidlugash I think one common use case is to use circle-radius to visualize some quantitative value (e.g. "population"). In that kind of case I think there's a strong incentive to use circle-pitch-alignment: viewport because distorting the shape of the circle based on pitch makes it harder to compare the relative sizes of two different circles.

I tried changing the style to circle-pitch-alignment: map for our data-driven circle example, and it wasn't obvious to me that it was a better default. To me, aligning to the map makes the data feel a little bit more like it's in the "background":

viewport aligned

screenshot 2017-06-26 13 50 12

map aligned

screenshot 2017-06-26 13 50 42

@nickidlugash
Copy link

I think one common use case is to use circle-radius to visualize some quantitative value (e.g. "population"). In that kind of case I think there's a strong incentive to use circle-pitch-alignment: viewport because distorting the shape of the circle based on pitch makes it harder to compare the relative sizes of two different circles.

Based on this description, I think you're talking about circle-pitch-alignment: viewport with circle-pitch-scale: viewport compared to circle-pitch-alignment: map with circle-pitch-scale: viewport? My personal opinion on this type of example would probably be to use circle-pitch-alignment: map with circle-pitch-scale: map because viewport alignment/size would distort the relative size of the circles to the basemap and relative distance between two circles, which distorts relative density. But you're probably right that we should just support all combinations because we don't know all the use cases people may have.

it might make sense to add a new (default) auto option to circle-pitch-scaling and make it key off of circle-pitch-alignment?

I think this makes sense.

@jfirebaugh
Copy link
Contributor

add a new (default) auto option to circle-pitch-scaling and make it key off of circle-pitch-alignment

This would mean that the default behavior of one of the two properties would have to change, right?
If the default for circle-pitch-alignment changes to map, then that's changing the default behavior for pitch alignment. If the default for circle-pitch-alignment stays viewport, but the default for circle-pitch-scaling changes to auto, then that's changing the default behavior for pitch scaling.

I'm pretty sure we don't want the latter, based on #2541. Making circles both align and scale with pitch makes the most sense, but is also technically a breaking change. Probably acceptable for GL JS, but less so the native SDKs. Thoughts?

@ChrisLoer
Copy link
Contributor Author

This would mean that the default behavior of one of the two properties would have to change, right?

Yeah, you're right, I forgot that circle-pitch-scale currently defaults to map. I would definitely want to avoid changing existing default behavior unless we had a strong reason. Since the current default behavior is scale: map, align: viewport, I wouldn't want to remove that option either (and I think it's a sensible choice when you want the circles to maintain size relative to their surroundings, but you also want them to look like annotations on top of the map, rather than part of the base map).

So: keep it the way it is in this PR, with all four combinations allowed?

@nickidlugash
Copy link

Making circles both align and scale with pitch makes the most sense, but is also technically a breaking change.

I don't know enough to weigh in on the acceptability of a breaking change here, but I do agree that this combination of defaults seems to make the most sense.

@peterqliu do you have opinions on default values for circle-pitch-alignment and circle-pitch-scale?

@peterqliu
Copy link
Contributor

I don't know enough to weigh in on the acceptability of a breaking change here,

I sense that we're early enough in our adoption here that we can do either without too huge a cost 👍

@peterqliu do you have opinions on default values for circle-pitch-alignment and circle-pitch-scale?

There's a great question here, because circles can be used as both

  • markers that denote a point on the map, possibly clickable/hoverable. In this use case, we want markers to align with viewport, so that they can be fully interactable even far away in a tilted map. This is especially important to maintain a sufficiently large hit area for clicking/fat-finger tapping.

  • circles that denote a circular area on the map, corresponding to a circular area in physical space. In this case, we want this to scale and align with the map, so that they look correct in real-world space, regardless of the camera.

Since circles are sized in pixels instead of meters, they're poorly adapted to represent round areas in real space, which diminishes the second use case. In this context, I lean toward both scaling and aligning to viewport, by default.

@1ec5
Copy link
Contributor

1ec5 commented Jun 27, 2017

Since circles are sized in pixels instead of meters, they're poorly adapted to represent round areas in real space, which diminishes the second use case.

At one point, we experimented with using circle layers to represent cul-de-sacs (turning circles) in Mapbox Streets. For this use case, the circle is an abstraction of a circular area in physical space. By analogy, line layers represent roadways in Streets even though line-width is measured in pixels rather than meters.

That said, I agree that circle layers are better suited for point features than area features in general. After all, the cul-de-sac use case doesn’t generalize to much larger areas, for which a real geographic circle geometry is a frequent request (cf. mapbox/mapbox-gl-native#2167).

@ChrisLoer
Copy link
Contributor Author

In this context, I lean toward both scaling and aligning to viewport, by default.

That argument makes sense, but reverses our last decision on these defaults... @samanpwbb, since you filed #2541 -- do you think if you were doing the same thing today and found the circles unscaled/pitched by default, it would be straightforward to discover the circle-alignment: pitch property?

@peterqliu
Copy link
Contributor

That argument makes sense, but reverses our last decision on these defaults

donning the circle-as-point-features lens again, I think a default of align:viewport is the more critical one, because it ensures that we'll see the full circle at all angles, and not just a skinny ellipse. scale:map is acceptable: it would mean smaller circles at a distance, but at least we'll see and interact with the whole ⚪️, and not just a 🕳

@ChrisLoer
Copy link
Contributor Author

So it sounds like the leading alternative would be to make scale default to auto (keyed off of alignment) and make alignment default to viewport. Given that @peterqliu thinks the current default (align: viewport; scale: map) is "acceptable", I'm inclined to keep it that way in the interest of stability across GL JS versions.

@jfirebaugh If that sounds good to you, could you review the rest of the PR for correctness? If you lean strongly towards the alternative default (align: viewport; scale: auto), let me know and I'll update the PR accordingly.

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

👍

@ChrisLoer ChrisLoer merged commit c4ae954 into master Jul 3, 2017
@ChrisLoer ChrisLoer deleted the cloer_circle_pitch_alignment branch July 3, 2017 23:22
lora-reames added a commit to lora-reames/DefinitelyTyped that referenced this pull request Aug 28, 2017
Add new icon-pitch-alignment and circle-pitch-alignment properties mapbox/mapbox-gl-js#4869 mapbox/mapbox-gl-js#4871
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants