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

[iOS - Google Maps] Fix animateToCoordinate and animateToRegion #1115

Conversation

ryankask
Copy link
Contributor

This addresses #955 and #811 by doing the following:

Google Maps requires that the QuartzCore framework is linked against the project so I assume it's fine to use CATransaction.

@ryankask ryankask force-pushed the google-maps-ios-animate-to-methods branch from 8a9778d to ac26f09 Compare March 12, 2017 10:43
@ryankask ryankask changed the title Fix Google Maps iOS animate* methods [iOS - Google Maps] Fix animateToCoordinate and animateToRegion Mar 12, 2017
@peterept
Copy link

Hi @ryankask brilliant mate - great catch on duration not working - Embarrassingly I assumed the animation code I copied from animateToRegion worked..

So I pulled this PR down and ran it with the following test page with a duration as 1500ms:

import React, { Component } from 'react';
import {
  AppRegistry,
  StyleSheet,
  Button,
  View,
  Text
} from 'react-native';
import MapView from 'react-native-maps'
import CustomCallout from './CustomCallout';

export default class HelloMaps extends Component {
  state = {
    markers: [
      { key: "test", coordinate: {latitude: -33.852896, longitude: 151.210291}}
    ]
  }
  render() {
    return (
      <View style={styles.container} >
      <MapView ref={component => this.map = component} style={styles.map} 
          provider={MapView.PROVIDER_GOOGLE}
      >
          {this.state.markers.map(marker => (
            <MapView.Marker
              title={marker.key}
              image={require('./assets/pin-comment.png')}
              key={marker.key}
              coordinate={marker.coordinate}
            >
              <MapView.Callout tooltip style={styles.customView}>
                <CustomCallout>
                  <Text>This is a custom callout bubble view</Text>
                </CustomCallout>
              </MapView.Callout>
            </MapView.Marker>            
          ))}
      </MapView>
      <Button 
        title="Go Sydney" 
        onPress={() => { this.map.animateToCoordinate({latitude: -33.852896, longitude: 151.210291}, 1500)  }}
      />
      </View>
    );
  }
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: 'flex-end',
    alignItems: 'center',
    backgroundColor: '#F5FCFF',
  },
  map: {
    ...StyleSheet.absoluteFillObject,
  },  
  customView: {
    width: 140,
    height: 100,
  },  
});

AppRegistry.registerComponent('HelloMaps', () => HelloMaps);

Just to confirm with my fix it animates in about 1 sec (as you discovered):

Imgur

Now if I run with your fix:

Imgur

Expected: Animates for 15 seconds
Actual: Animated for ~4 seconds. The animation is 4 times faster then expected.

Note: Same behaviour in both IOS Simulator (iPhone 6) and real device (iPhone 5).
Xcode version 8.1

On playing around if I use the duration 1500*4, then it runs at 15 seconds.

I have searched around and I can't see why this is happening! :-(

Small nit, a bit cleaner to read rather then:
[CATransaction setValue:[NSNumber numberWithFloat: duration/1000] forKey:kCATransactionAnimationDuration];
use: [CATransaction setAnimationDuration:duration/1000];

@ryankask ryankask force-pushed the google-maps-ios-animate-to-methods branch from ac26f09 to a8c6a2c Compare March 15, 2017 12:26
@ryankask
Copy link
Contributor Author

Hi @peterept. Thanks for pointing out the setAnimationDuration method. It's much nicer and I've updated the PR.

In your example, can you change the duration to 15000 (note the extra 0) to check if it animates for 15 seconds? We're going from millisecond durations in the JS world to durations in seconds. The video seems like 1.5 seconds to me although it is bit tricky with tiles loading after.

@peterept
Copy link

Ahh yes that was it @ryankask ! !

I was staring at the 1500 for an hour and didn't notice it should have been 15000. Thank you for spotting that.

Confirmed, 15000 animates correctly for 15 seconds:

animateToCoordinate imgur

Also checked animateToRegion with this.map.animateToRegion({latitude: -33.852896, longitude: 151.210291, latitudeDelta: 0.0025, longitudeDelta: 0.0025}, 15000), and it correctly works:

animateToRegion imgur

Both work and smooth as silk on simulator and device.

Your PR lgtm.

Nice one, this will improve my app!

@peterept
Copy link

@ryankask how do we get this merged?

@grundmanise
Copy link

Can fitToElements, fitToSuppliedMarkers, fitToCoordinates, also have a duration prop?

@ryankask
Copy link
Contributor Author

ryankask commented Mar 23, 2017

@grundmanise They might be able to but at the moment the only two methods on MapView that take a duration argument are the ones addressed in this PR.

@peterept I'm not sure. Perhaps next time the project maintainers comb through the PRs they can comment.

At the moment I just created a new category in AIRGoogleMapManager+Animation.m to add the animateToCoordinate method.

@grundmanise
Copy link

Thanks for the answer.

Use Core Animation for animateToRegion(). This is backwards incompatible because the default duration of 500ms was being ignored before but now works.

You mean that now animateToRegion ignores duration prop when PROVIDER_GOOGLE?

@ryankask
Copy link
Contributor Author

ryankask commented Mar 23, 2017

If I've observed correctly, it doesn't ignore it but it tries to use it with an iOS animation method that doesn't work. The effect is that it's ignored.

@seppemarotta
Copy link

seppemarotta commented Mar 25, 2017

I'm so exited to add this to my project! Thxs guys for your time!

@lelandrichardson
Copy link
Collaborator

@ryankask i'd like to get this in... do you think you can rebase? I've changed some of the project structure around and it's causing conflicts

@ryankask ryankask force-pushed the google-maps-ios-animate-to-methods branch from a8c6a2c to 62f6de5 Compare March 27, 2017 07:39
@ryankask
Copy link
Contributor Author

@lelandrichardson I've rebased the branch. There weren't any conflicts but I wasn't able to run the example project.

There are some instructions for how to run the example project here: https://github.com/airbnb/react-native-maps/blob/master/docs/examples-setup.md. It looks like the npm install part needs to be tweaked because of the reorganisation but what about the Cocoapods part?

I'm seeing the below but I haven't had a chance to look into it:

2017-03-27 at 09 31

@guilhermepontes
Copy link
Contributor

any updates on this?

@mbroadst
Copy link

@lelandrichardson just ran into this problem myself, is there anything that can be done to expedite merging this?

@sonaye
Copy link

sonaye commented Apr 27, 2017

Been waiting for this one to get merged for a while.

@terribleben
Copy link
Contributor

Just chiming in that we've verified this fix on Expo and are pulling it into our next SDK version.

@peterept
Copy link

Thanks @terribleben! (and of course @ryankask for the solution) 😀

@christopherdro christopherdro merged commit 0c92524 into react-native-maps:master Jun 15, 2017
@ngandhy
Copy link

ngandhy commented Jun 16, 2017

@ryankask - Thanks!! Works great :)

yosimasu pushed a commit to yosimasu/react-native-maps that referenced this pull request Jun 19, 2017
…t-native-maps#1115)

* Add animateToCoordinate to Google Maps on iOS

* Add animate to random coordinate button in example app

* Fix animateToRegion duration for Google Maps on iOS
yosimasu pushed a commit to yosimasu/react-native-maps that referenced this pull request Jun 19, 2017
…t-native-maps#1115)

* Add animateToCoordinate to Google Maps on iOS

* Add animate to random coordinate button in example app

* Fix animateToRegion duration for Google Maps on iOS
sorodrigo pushed a commit to Vizzuality/react-native-maps that referenced this pull request Jul 6, 2017
…-upstream

* upstream/master:
  Add minZoom and maxZoom properties for android and ios (react-native-maps#1360)
  Reference install solution in issue react-native-maps#718 in install docs (react-native-maps#1448)
  updates npm cache clean command (react-native-maps#1450)
  v0.15.3
  Added BatchedBridge
  Upgraded ios deps
  Use prop-types and add supprort for RN 0.45
  Allow react 16.0.0-alpha
  [Android] Code cleanup step I - reformatting (react-native-maps#1415)
  Fixes google map null pointer exception (react-native-maps#1403)
  [iOS - Google Maps] Fix animateToCoordinate and animateToRegion (react-native-maps#1115)
  Update from View.propTypes to ViewPropTypes to match RN v0.44.0 (react-native-maps#1323)
  Fix import header for React Native 0.44.2 (react-native-maps#1362)
  Fix a couple typos (react-native-maps#1375)
j8seangel added a commit to Vizzuality/react-native-maps that referenced this pull request Jul 14, 2017
* 'master' of github.com:Vizzuality/react-native-maps:
  fix: error syntax on AirMaps max and min zoom level not nil check
  Add minZoom and maxZoom properties for android and ios (react-native-maps#1360)
  Reference install solution in issue react-native-maps#718 in install docs (react-native-maps#1448)
  updates npm cache clean command (react-native-maps#1450)
  v0.15.3
  Added BatchedBridge
  Upgraded ios deps
  Use prop-types and add supprort for RN 0.45
  Allow react 16.0.0-alpha
  [Android] Code cleanup step I - reformatting (react-native-maps#1415)
  Fixes google map null pointer exception (react-native-maps#1403)
  [iOS - Google Maps] Fix animateToCoordinate and animateToRegion (react-native-maps#1115)
  Update from View.propTypes to ViewPropTypes to match RN v0.44.0 (react-native-maps#1323)
  Fix import header for React Native 0.44.2 (react-native-maps#1362)
  Fix a couple typos (react-native-maps#1375)
pjaraherrera pushed a commit to pjaraherrera/react-native-maps that referenced this pull request Sep 27, 2017
…t-native-maps#1115)

* Add animateToCoordinate to Google Maps on iOS

* Add animate to random coordinate button in example app

* Fix animateToRegion duration for Google Maps on iOS
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.