maps icon indicating copy to clipboard operation
maps copied to clipboard

feat: implement `MovePointShapeAnimator` and `ChangeLineOffsetsShapeAnimator`

Open naftalibeder opened this issue 1 year ago • 15 comments

Description

This implements the specific shape animators for animating a single point source and a line by its offsets.

Checklist

  • [x] I've read CONTRIBUTING.md
  • [x] I updated the doc/other generated code with running yarn generate in the root folder
  • [x] I have tested the new feature on /example app.
    • [x] In V11 mode/ios
    • [x] In New Architecture mode/ios
    • [x] In V11 mode/android
    • [x] In New Architecture mode/android
  • [x] I added/updated a sample - if a new feature was implemented (/example)

Videos

https://github.com/rnmapbox/maps/assets/2423291/4fba6b9c-c8e4-47cd-ad09-c636fff18693

https://github.com/rnmapbox/maps/assets/2423291/d94b3651-5523-4b11-a6c4-e793a9a841b0

naftalibeder avatar Dec 19 '23 21:12 naftalibeder

@mfazekas Is there a non-obvious requirement for registering a new module? I think I've exactly duplicated the MovePoint animator, and I've re-run yarn install and restarted the bundler, but no matter what I do, I get

Invariant Violation: TurboModuleRegistry.getEnforcing(...): 'NativeRNMBXChangeLineOffsetsShapeAnimatorModule' could not be found. Verify that a module by this name is registered in the native binary.

EDIT: Never mind. Typo :(

naftalibeder avatar Dec 21 '23 16:12 naftalibeder

@mfazekas This is pretty much feature-complete for iOS. Thoughts or concerns before porting to Android? Updated videos at the top.

naftalibeder avatar Dec 22 '23 21:12 naftalibeder

@mfazekas I think this is getting close.

One question: How would you like to handle an animator stopping and starting? Right now, the implementations are different on iOS and Android.

On iOS, an animator stops its timer when there are no more consumers subscribed to it.

On Android, an animator stops its timer when the subclass's getAnimatedShape() implementation returns false in the second tuple element.

I'm not sure I understand the intention with the Android version, but I'm curious what your thoughts are here.

naftalibeder avatar Jan 16 '24 16:01 naftalibeder

@mfazekas I think this is getting close.

One question: How would you like to handle an animator stopping and starting? Right now, the implementations are different on iOS and Android.

On iOS, an animator stops its timer when there are no more consumers subscribed to it.

On Android, an animator stops its timer when the subclass's getAnimatedShape() implementation returns false in the second tuple element.

I'm not sure I understand the intention with the Android version, but I'm curious what your thoughts are here.

The intention there was that an animation would return false if it reached the final state. Then it doesn't make sense to continue the animation. Like if the animation is to move a shape left with 1degree over 10 seconds, after the shape moved by 1degree then it's finished and no need to continue.

I guess if no listeners we can also stop the animation, the timer needs to stop for sure.

I think if we don't want to export start/stop which is fine for first versions, then just stopping the timer (and animation) when either there are no listeneres or the enimation reached the goal state sounds fine

mfazekas avatar Jan 16 '24 17:01 mfazekas

@mfazekas I'm pretty sure this is ready for a final review! I've updated the videos in the description.

The only known bug is on Android with new architecture enabled. The @rneui Slider component triggers an error, apparently due to the value number being passed losing precision when cast to Long on the native side. It seems like a bug in React Native (should be a debug log, not an error?), or maybe in the component library. But it has nothing to do with the map-related functionality.

naftalibeder avatar Jan 30 '24 20:01 naftalibeder

@mfazekas I think I took care of all your feedback. Anything else you see?

naftalibeder avatar Feb 02 '24 20:02 naftalibeder

@mfazekas Changes made. :) What's next?

naftalibeder avatar Feb 05 '24 16:02 naftalibeder

@mfazekas Changes made. :) What's next?

Fix CI

mfazekas avatar Feb 05 '24 18:02 mfazekas

@mfazekas Ok seems good to go.

naftalibeder avatar Feb 05 '24 20:02 naftalibeder

@mfazekas Ok seems good to go.

There are some errors in iOS builds

mfazekas avatar Feb 06 '24 06:02 mfazekas

@naftalibeder sorry can you update the PR? The CI was broken and should be fixed now:

@Cdingram can you update your PR, CI was broken and was fixed with this commit https://github.com/rnmapbox/maps/pull/3361/commits/5394b31ebd4a6e9b064b504b1d00471be57805b1 so

mfazekas avatar Feb 07 '24 19:02 mfazekas

@mfazekas Any ideas about the CI failures? They seem outside of the scope of what I've worked on in this PR, so I'm not sure what to look into.

naftalibeder avatar Feb 12 '24 21:02 naftalibeder

@mfazekas Any ideas about the CI failures? They seem outside of the scope of what I've worked on in this PR, so I'm not sure what to look into.

I think it's related to issue cocoa pods 1.15.0. I'll add a workaround for this on the main branch.

mfazekas avatar Feb 13 '24 06:02 mfazekas

@mfazekas So what is left on this before you'd be comfortable merging it?

naftalibeder avatar Feb 21 '24 15:02 naftalibeder

@mfazekas So what is left on this before you'd be comfortable merging it?

The biggest issue is that all iOS CI fails. I don't see the error message.

mfazekas avatar Feb 22 '24 05:02 mfazekas

@mfazekas All checks passed :)

naftalibeder avatar Feb 22 '24 17:02 naftalibeder