-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Expose native worklet API #1790
Expose native worklet API #1790
Conversation
This reverts commit 1cdb5c9.
`CADisplayLink::targetTimestamp` is only available on iOS 10 or newer. So it wouldn't even build.
it compiles 🎉 |
I've refactored the code so that other libraries can inherit
While everything compiles correctly (and uses the same semantics, so everything REA related works as expected), I'm not sure what's missing here because when I call I'd appreciate any feedback from you guys, maybe you can spot an error in my code - or let me know if there's any additional work required for this to work. cc @Szymon20000 @karol-bisztyga @piaskowyk @terrysahaidak @kmagiera |
Managed to get it working successfully: Screen.Recording.2021-03-11.at.10.54.51.movThis means, that this PR is technically ready. I'd like you guys to answer my questions in the comment above on how we're going to proceed with this in terms of code organisation (regarding the valueSetter errorHandler and scheduler), but everything works now. 🔥 |
As a side note, I noticed that you guys did a lot of std::shared_value<T> someValue(new T(...)); instead of std::shared_value<T> someValue = std::make_shared<T>(...); is there a specific reason for that? Might be a micro-optimization, but EDIT: Created a separate PR to replace all |
This automatically transforms the `useFrameProcessor` hook from my VisionCamera.
I made a multithreading library by using the changes in this PR: https://github.com/mrousavy/react-native-multithreading |
I noticed that I can't simply link the Android Reanimated native code against my library because it is pre-built using an .aar, so this whole "Expose native worklet API" only works on iOS atm. Since react-native is starting to use more and more C++ code (Rearchitecture, TurboModules), and with the release of RN 0.64 even requires the user to install the NDK (see template's build.gradle), I think it wouldn't be too far off to not use a prebuilt .aar library for reanimated, but instead ship the .h/.cpp sources and let the user compile it themselves. (That is no extra effort for the user, Android/Gradle manages all of that automatically.) |
Closing in favor of #1861 |
@@ -23,6 +23,8 @@ const functionArgsToWorkletize = new Map([ | |||
['withSpring', [2]], | |||
['withDecay', [1]], | |||
['withRepeat', [3]], | |||
// for https://github.com/cuvent/react-native-vision-camera | |||
['useFrameProcessor', [0]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a possibility to pass additional hooks/functions as options in babel.config.js. https://babeljs.io/docs/en/plugins/#plugin-options
It cannot be directly in the plugin code as it's not part of reanimated.
Description
This PR exposes the native "worklet" API by lifting the "manage-a-runtime" logic out of
NativeReanimatedModule
so that other native modules can implement this for their own use case. This "manage-a-runtime" logic is implementable by theRuntimeManager
base class.Advantages
NativeReanimatedModule
, making everything a bit more "decoupled"Related
Approach
I will be starting from this location in my VisionCamera project, I am trying to create a worklet here.
So
ShareableValue::adapt
is my starting point, from there on I will start to remove allNativeReanimatedModule
references and will try to decouple it as much as possible.Solution
The current solution works fine and nothing has changed for Reanimated. No side-effects whatsoever (afaik).
But I am not 100% happy with one thing:
RuntimeManager
has a Scheduler, ValueSetter and ErrorHandler which are all properties that are really specific to Reanimated'sNativeReanimatedModule
. Ideally other native modules shouldn't be using those 3 things, since ValueSetter is specific for mirroring styles from native to JS (afaik), ErrorHandler just displays the errors in worklets (afaik) and scheduler is responsible for dispatching from Worklet -> JS and vice versa. Ideally I want to also abstract those away with the following solution:errorHandler
is never needed, the default React LogBox should always be used for any errors in worklets and should even display source frames. For any other calls, just throw anstd::exception
(orjsi::JSError
)valueSetter
is not needed at all, this is only needed for Reanimated. Would be better to remove that fromRuntimeManager
too.scheduler
is needed to userunOnJS
I believe, no idea if we really need that. It's also really hard to create an instance of theScheduler
on Android...@Szymon20000 I would like your opinion on those things. I guess we can merge this PR since no side effects are introduced and it's now possible for other libraries to hook into this logic and concentrate on those 3 things in another PR. Ideally it should be really simple to create worklets, but now it requires you to create an ErrorHandler and Scheduler.
Edit
As for the
scheduler
, I haven't tried it yet but what happens if I do the followng:sv.value = 5
will be executed in my customjsi::Runtime
from the Frame Processor, so it will not be updated on the UI thread. Would this still work? Is this like updating the value on the JS thread?