maps
maps copied to clipboard
feat: implement `MovePointShapeAnimator` and `ChangeLineOffsetsShapeAnimator`
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
@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 :(
@mfazekas This is pretty much feature-complete for iOS. Thoughts or concerns before porting to Android? Updated videos at the top.
@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.
@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 returnsfalse
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 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.
@mfazekas I think I took care of all your feedback. Anything else you see?
@mfazekas Changes made. :) What's next?
@mfazekas Changes made. :) What's next?
Fix CI
@mfazekas Ok seems good to go.
@mfazekas Ok seems good to go.
There are some errors in iOS builds
@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 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.
@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 So what is left on this before you'd be comfortable merging it?
@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 All checks passed :)