refs #6779: mobile SDK style transition options #7711

incanus · 2017-01-13T03:26:19Z

Starting to plumb the core transition options for the style out to iOS and eventually Android.

Duration works fine, but delay causes a crash within Interpolator<Color> due to assert in the Color constructor failing, in my case due to negative RGBA values (going from gray to black buildings). Delay probably isn't factored in when continuing to subtract below zero. This may be a core bug.

Fixes #6779.

mention-bot · 2017-01-13T03:26:21Z

@incanus, thanks for your PR! By analyzing this pull request, we identified @1ec5, @boundsj and @friedbunny to be potential reviewers.

1ec5 · 2017-01-13T05:10:33Z

platform/darwin/src/MGLStyle.h

+- (void)setTransitionDuration:(NSTimeInterval)duration;
+- (NSTimeInterval)transitionDuration;
+- (void)setTransitionDelay:(NSTimeInterval)delay;
+- (NSTimeInterval)transitionDelay;


These options should be exposed as properties.

1ec5 · 2017-01-13T05:12:22Z

platform/darwin/src/NSDate+MGLAdditions.h

@@ -7,4 +7,7 @@
 /// Converts from a duration in seconds to a duration object usable in mbgl.
 mbgl::Duration MGLDurationInSeconds(NSTimeInterval duration);

+/// Converts from an mbgl duration object to a duration in seconds.
+NSTimeInterval MGLSecondsFromDuration(mbgl::Duration duration);


MGLTimeIntervalFromDuration() would make the return type clearer, presuming that an NSTimeInterval is always measured in seconds (which is the case).

Then should we not also rename MGLDurationInSeconds() to MGLDurationFromTimeInterval()?

MGLDurationInSecondsFromTimeInterval(), actually. mbgl::Duration can be expressed in many units; seconds is the one we’ve chosen to use for consistency with NSTimeInterval, but other parts of mbgl use milliseconds instead.

Right, took me a while to figure that out, but true. Do you think it's worth adding an assertion or other check here that it remains in seconds in this part of the code?

No, I don’t think that’s an issue with std::chrono::duration or mbgl::Duration (which is actually in nanoseconds, my bad). MGLDurationInSeconds() performs a duration_cast to ensure that the output has the right quantity (if not the same unit as the input).

mbgl::Duration is similar to NSTimeInterval in the sense that the unit is fixed and built into the type. You can't have one mbgl::Duration representing a duration "in seconds" and another representing a duration "in milliseconds". The actual unit is defined by std::chrono::steady_clock and I'm not sure that it's invariant across platforms or standard library implementations. But it is fixed at compile time.

1ec5 · 2017-01-13T05:14:33Z

platform/darwin/src/MGLStyle.mm

+
+- (void)setTransitionDuration:(NSTimeInterval)duration
+{
+    mbgl::style::TransitionOptions transitionOptions = self.mapView.mbglMap->getTransitionOptions();


auto is typically used in situations like this to avoid repetition.

incanus · 2017-01-13T18:58:42Z

I'm going to bring this to Android too, for parity.

1ec5 · 2017-01-13T19:08:24Z

delay causes a crash within Interpolator<Color> due to assert in the Color constructor failing, in my case due to negative RGBA values (going from gray to black buildings). Delay probably isn't factored in when continuing to subtract below zero. This may be a core bug.

Can you file a separate issue about that? Depending on how systematic that issue is, we may want to hold off on a delay option until it’s fixed.

There’s an outstanding PR to support realistic color interpolation in style properties: #6442. (#6724 and #6721 track the SDK frontend to this support.) Does the style specification need something similar for global transitions?

incanus · 2017-01-13T20:23:41Z

Can you file a separate issue about that? Depending on how systematic that issue is, we may want to hold off on a delay option until it’s fixed.

👉 #7720

…tion-options

incanus · 2017-01-17T20:16:58Z

Delay now works fine when rolling in the fix for #7720 from #7756.

Working on Android next.

…on-options

incanus · 2017-01-18T00:59:54Z

@cammace Any chance you could review the Android part of this? I'm also all 👂 when it comes to possible testing ideas. I checked out some other tests for animated components like camera changes, but from what I could see, none of them actually try a non-zero duration?

1ec5 · 2017-01-18T01:12:56Z

platform/ios/src/MGLMapView.mm

@@ -1226,7 +1226,7 @@ - (void)handlePanGesture:(UIPanGestureRecognizer *)pan
        if (drift)
        {
            CGPoint offset = CGPointMake(velocity.x * self.decelerationRate / 4, velocity.y * self.decelerationRate / 4);
-            _mbglMap->moveBy({ offset.x, offset.y }, MGLDurationInSeconds(self.decelerationRate));
+            _mbglMap->moveBy({ offset.x, offset.y }, MGLDurationInSecondsFromTimeInterval(self.decelerationRate));


You’ll need to make corresponding changes to the macOS implementation of MGLMapView.

1ec5

The iOS and macOS changes look good. Some suggested wording below. Don’t forget to update the iOS and macOS changelogs.

1ec5 · 2017-01-18T02:11:07Z

platform/darwin/src/MGLStyle.h

+#pragma mark Managing a Style’s Transition Options
+
+/**
+ Animation duration for style changes. Defaults to zero (instant change).


What kind of changes can be animated? Just style layer attributes?

The duration (in seconds) to animate any changes to layout or paint attributes in the style.

By default, this property is set to 0 seconds, so any changes take effect without animation.

1ec5 · 2017-01-18T02:11:52Z

platform/darwin/src/MGLStyle.h

+@property (nonatomic) NSTimeInterval transitionDuration;
+
+/**
+ Animation delay for style changes. Defaults to zero (instant change).


The amount of time (in seconds) to wait before animating any changes to layout or paint attributes in the style.

By default, this property is set to 0 seconds, so any changes begin to animate immediately.

…on-options

cammace

Don’t forget to also update the android changelog :)

1ec5 · 2017-01-19T11:04:15Z

platform/darwin/src/MGLStyle.h

+#pragma mark Managing a Style’s Transition Options
+
+/**
+ The duration in seconds to animate any changes to the style URL or to layout and paint attributes.


Huh, I overlooked the fact that changes to the style URL can also be animated. That’s kind of awkward, since changing the style URL means swapping one MGLStyle object for another.

Actually, #5665 makes it sound like we don’t have “diff and patch” functionality in mbgl yet. Has the situation changed since July?

Updated changelogs to mention #7446, #7356, #7465, #7616, #7445, #7444, #7526, #7586, #7574, and #7770. Also corrected the blurb about #7711.

refs #6779: first cut of iOS transition options

aeb20cb

incanus added Android Mapbox Maps SDK for Android feature iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS runtime styling labels Jan 13, 2017

incanus added this to the ios-future milestone Jan 13, 2017

incanus self-assigned this Jan 13, 2017

1ec5 modified the milestones: ios-v3.5.0, ios-future Jan 13, 2017

1ec5 removed the Android Mapbox Maps SDK for Android label Jan 13, 2017

1ec5 reviewed Jan 13, 2017

View reviewed changes

incanus added 2 commits January 13, 2017 10:55

use properties instead

b2b0198

use auto

705cb6b

incanus added the Android Mapbox Maps SDK for Android label Jan 13, 2017

incanus mentioned this pull request Jan 13, 2017

Crash transitioning color with delay #7720

Closed

jfirebaugh and others added 3 commits January 17, 2017 11:48

[core] Fix calculation of delayed transitions

fb9c416

Merge remote-tracking branch 'origin/fix-7720' into 6779-style-transi…

59f5e7a

…tion-options

re-enable transition delay due to #7756 fix

2c68ae7

incanus added 3 commits January 17, 2017 16:27

Merge remote-tracking branch 'origin/master' into 6779-style-transiti…

2f4ee9c

…on-options

pipe Android support

16d6a05

docs comments

530e32b

incanus requested a review from cammace January 18, 2017 00:59

clearer function naming

6ddc216

1ec5 reviewed Jan 18, 2017

View reviewed changes

incanus added 3 commits January 17, 2017 17:21

update macos for new function

1fa75dd

exercise macos transition options

71947b6

upping checkstyle max file length to 2042 per chat with @zugaldia

264005b

1ec5 approved these changes Jan 18, 2017

View reviewed changes

incanus added 2 commits January 18, 2017 11:00

minor comment clarity tweaks

d2d4af9

Merge remote-tracking branch 'origin/master' into 6779-style-transiti…

37f3c54

…on-options

cammace approved these changes Jan 18, 2017

View reviewed changes

update Android, iOS, and macOS changelogs

6745984

1ec5 mentioned this pull request Jan 18, 2017

Update iOS, macOS changelogs #7731

Merged

incanus merged commit f671569 into master Jan 18, 2017

incanus deleted the 6779-style-transition-options branch January 18, 2017 20:43

1ec5 reviewed Jan 19, 2017

View reviewed changes

1ec5 added a commit that referenced this pull request Jan 19, 2017

[ios, macos] Updated changelogs

5c144a2

Updated changelogs to mention #7446, #7356, #7465, #7616, #7445, #7444, #7526, #7586, #7574, and #7770. Also corrected the blurb about #7711.

1ec5 mentioned this pull request Jan 25, 2017

Per-property transitions are not parsed from style #7863

Closed

1ec5 mentioned this pull request Feb 9, 2017

Per-attribute transition properties on MGLStyleLayer #8010

Closed

1ec5 mentioned this pull request Mar 3, 2017

NSDate(MGLAdditions) turns seconds into nanoseconds #8264

Closed

tobrun mentioned this pull request Mar 30, 2017

TransitionOptions conversion when set #8575

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refs #6779: mobile SDK style transition options #7711

refs #6779: mobile SDK style transition options #7711

incanus commented Jan 13, 2017

mention-bot commented Jan 13, 2017

1ec5 Jan 13, 2017

incanus Jan 13, 2017

1ec5 Jan 13, 2017

incanus Jan 13, 2017

1ec5 Jan 13, 2017

incanus Jan 13, 2017

1ec5 Jan 13, 2017

jfirebaugh Jan 16, 2017

incanus Jan 18, 2017

1ec5 Jan 13, 2017

incanus Jan 13, 2017

incanus commented Jan 13, 2017

1ec5 commented Jan 13, 2017

incanus commented Jan 13, 2017

incanus commented Jan 17, 2017

incanus commented Jan 18, 2017

1ec5 Jan 18, 2017

incanus Jan 18, 2017

1ec5 left a comment

1ec5 Jan 18, 2017

1ec5 Jan 18, 2017

cammace left a comment

1ec5 Jan 19, 2017

1ec5 Jan 19, 2017

refs #6779: mobile SDK style transition options #7711

refs #6779: mobile SDK style transition options #7711

Conversation

incanus commented Jan 13, 2017

mention-bot commented Jan 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incanus commented Jan 13, 2017

1ec5 commented Jan 13, 2017

incanus commented Jan 13, 2017

incanus commented Jan 17, 2017

incanus commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cammace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment