-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[wrap) #1363
[wrap) #1363
Conversation
But why? |
For consistency with mapbox/mapbox-gl-native#1829, which was necessary because functions that return wrapped values need predictable values. Currently, wrapping 360 to between 0 and 360 gives you 360, but wrapping 720 or -360 to the same range gives you 0. We could fix it so that any multiple of the maximum returns the maximum, but it's still counterintuitive because then negative multiples of the maximum don't behave the same way as their positive counterparts. |
It just looks weird because you change a working util method in a way that introduces code duplication in lots of places. Leaflet solves this problem by having boolean |
Can I merge this? The current
AFAICT we use it only to wrap the bearing value. But I was also hoping to use it in a fix for #1483 |
@bhousel does it fix the issue? |
Yes, Minh's wrap makes much more sense:
|
The code duplication is unfortunate. Could we just switch to wrapping longitude and bearing to [-180, 180)? Or maybe longitude to [-180, 180) and bearing to [0, 360). |
Also this needs to be rebased on master -- some tests have been added for |
@jfirebaugh sounds good to me. |
There are usually two reasons to use a wrap function:
On the other hand, if the wrap function were to switch on the sign of the minimum value, I’d consider that to be surprising, hidden behavior. So how about two functions, |
How about supporting either (n, min, max) or (n, max, min) argument order and always wrapping inclusive on the first extremum and exclusive on the second? |
That’s an elegant approach. But at the risk of bike-shedding on this issue, I’m pretty sure that if I read |
e892170
to
a096783
Compare
OK, I wound up implementing the approach I outlined in #1363 (comment). The upshot is that, going forward, you’ll probably use |
Looks good, but I'd rather have it as an optional |
a096783
to
0bb067e
Compare
Sorry for encouraging bikeshedding on this. I merged a minimal change -- just making the existing wrap behavior truly exclusive on min (so |
This means the bearing can never be 0°, which is a little strange. But functionally it’s all good. |
It can be 0°; gl-js uses a range of (-180°, 180°]. |
Got it; I was thrown off by the title of #1509. 😄 Thanks for taking care of this. |
wrap() now excludes the maximum value, returning a value in the range [min, max). wrap(max, min, max) now returns the same thing as wrap(max * 2, min, max).
If (wrap] is desired, it is the responsibility of the caller to handle the case in which min is returned.
Backported from mapbox/mapbox-gl-native#1829. No expected changes in behavior.
/cc @mourner