-
Notifications
You must be signed in to change notification settings - Fork 447
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
feat: skottie #657
feat: skottie #657
Conversation
/** | ||
Returns the underlying object from a host object of this type | ||
*/ | ||
static SkCanvas *fromValue(jsi::Runtime &runtime, |
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.
As the API design is to call animation.render(canvas, progress)
I needed to get the canvas as parameter when called from JS
Hi @hannojg!! First of all, thanks a bunch for submitting this PR - it is really enjoyable to see what developers do with RN Skia!
We've discussed this a bit and our initial feeling is that we really don't won't the size of the library to grow any further now, so if possible we'd love if this could be done as some kind of opt-in solution as you say. We'd be happy to help out to see how this can be done (see discussion below)
Sounds correct :) A HostObject is a Javascript C++ object that can be exposed in JavaScript, and we have some helper classes to create these with support for wrapping Skia objects - so this looks correct to me.
We don't have a hard dependency on Reanimated, so we'd like to help out implementing this with the built-in Skia Values - which should support the necessary functionality to run Lottie animations.
Awesome!
Maybe this can be solved through Skia Values (I can help here) and that we can wrap the needed automation inside the component itself.
Agree.
Maybe this is something we could do initially to verify if adding Skottie support is worth it?
I'm fine with the suggested implementation - we can look at the performance numbers to see what we need to optimise in the near future.
Don't think so :) My main comment to this PR is that it is very much welcome - but it opens up the infamous scope discussion. What should be the core features of React Native Skia be, and what should be delivered in additional packages depending on this package. We have received suggestions for supporting animated GIFs (#666), we support SVG (which might as well be extracted to a separate package) and support for drawing on video and camera streams #667 has been requested. Maybe we should look into how React Native Skia can support extensions like Skottie in sub packages in some way that could help both separate the features supported so it would be easier to keep apps small, and also to enable integration with other packages that can use Skia for drawing on camera- or video-streams etc. We will try to discuss this "internally" - but unfortunately not until the beginning of august due to a combination of tight schedules and holidays - if you have suggestions and/or questions to my input here - feel free to ask questions. Once again - thanks for this awesome PR!! |
@chrfalch Thanks for taking the time to go through the PR!
Totally agree on that, to me that would feel the cleanest, also in hindsight of other extensions to skia that want to be built.
Yes, I will do that next, maybe implement optimisations if the numbers don't seem promising enough to really see if it's worth further investigating! If so, I will implement the other things mentioned and it would be great if you guys could have another look then to help out e.g. with the hook for running the animation (will lyk). if the results look promising I'd say we leave this PR open until we find a suitable solution with opt-in or sub-packages. Until then I'd use this PR as a patch in our app. |
@hannojg I played with this branch a bit and it looks great. Does it sound like you may have been looking for help on the animation stuff? To loop the animation without using reanimated 2? This could easily be fixed. About the exact API to provide, we can discuss it. With @chrfalch, we will discuss these extra dependencies and also see if we want to align with the libraries shipped with CanvasKit. In the meantime, it would be good to check if there are use-cases where using Skottie over Lottie React Native has benefits. That being said, being having scrolled the Skottie source code quite a bit, I was surprised to see a 2MB overhead. The overhead size on Android or CanvasKit seems closer to the number of what I would have expected. Thank you for this awesome contribution, give us two or three weeks to get a good story there and we will move forward with this. Seeing the incredible things you have built with React Native, we can't wait to see where you will go with Skia. |
Hey 👋 I changed the example code to just use a skia value for driving the animation. ConculsionI draw the following conclusions from my testing:
To us that really means that Skottie can become helpful in our production app to improve UX on low-end devices. Will keep you updated how it goes (will use this PR) Testing dataExample screenshot of all animations rendered at the same time:
I used https://github.com/bamlab/react-native-flipper-performance-monitor and https://github.com/bamlab/android-performance-profiler for performance measurements. The branch used for testing can be found here (dirty): https://github.com/skillnation/react-native-skia/tree/feat/skottie-perf-measurements |
Thanks once again, @hannojg! Great work. We're on holiday this week, but will get back and discuss this PR when we're back. What we'd like to discuss is some kind of way to make features like this optional (or separate but taking advantage of the Skia builds we're creating) so that our users can select what features they want from the library. If you have any ideas or suggestions we'd be super happy! |
This PR adds support for the Skottie Module to react-native-skia.
Overview of changes
skia_enable_skottie=true
to include the skia module during the buildsksg
, so I need to import/copy that static library over as well next to skottieJsiSkSkottieAnimation
custom HostObject (correct term?), which is analogous to the Animation class in Skottie.hI will comment on the PR code in a moment to explain some changes in detail.
Changes I still need to do
I wanted to get feedback on this PR as early as possible. To get input quickly if I am heading in the wrong direction somewhere.
Please let me know if it makes sense for me to continue with this. I am also curious whether I screwed up the code/naming conventions somewhere, so please let me know so I can improve 😄
(My goal with this is to see if skottie can provide a better performance, especially on low end android devices, than lottie-android)