-
Notifications
You must be signed in to change notification settings - Fork 11
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
Merge vellottie
#13
Merge vellottie
#13
Conversation
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.
Lots of small things which need addressing, but overall this works well for me locally
I feel like there is a better way to present the downloads (e.g. in a clickable grid showing them all running at the same time?)
But that can be a follow-on, if you agree that would be good.
I've also seen that the downloads aren't included in the default runner by default. I'm not sure the best way to handle that is - we should probably not panic, but yes print the download message? |
Co-authored-by: Daniel McNab <[email protected]>
Co-authored-by: Daniel McNab <[email protected]>
Co-authored-by: Daniel McNab <[email protected]>
Co-authored-by: Daniel McNab <[email protected]>
Co-authored-by: Kaur Kuut <[email protected]>
Co-authored-by: Kaur Kuut <[email protected]>
This is rather large and I’m currently otherwise occupied, but at a glance, I don’t see any major blockers and I’ll review this by the of the week. Thanks @simbleau for taking this on! |
Co-authored-by: Daniel McNab <[email protected]>
I haven’t forgotten about this. I’ll be taking a look shortly. |
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.
Thanks Spencer! Overall this looks like a set of solid improvements. I added some notes on things I'd like changed before merging, suggestions for future work and a few random comments.
I'm happy to land this after the requested changes are addressed.
/// Easing tangent going into the next keyframe | ||
pub in_tangent: Option<EasingHandle>, | ||
/// Easing tangent leaving the current keyframe | ||
pub out_tangent: Option<EasingHandle>, | ||
/// Whether it's a hold frame. |
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.
Future work in a follow up: these drastically increase the size of this struct. In cases where tangents are non-existent or sparse, this can waste a lot of space. Skottie stores these in a side table and keeps an index in the actual time struct. We can do the same, packing tangent vec index and the hold bit in a single u32
, reducing the size of this to 8 bytes.
Basically, low bit is the hold flag and the high 31 bits store index + 1
where 0 represents no tangents.
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.
Can you link to the Skottie implementation for reference?
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.
Either way, I added this as a task to #14 - Will resolve this one.
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.
I’ll add a comment with the link to that issue.
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.
There’s a huge amount of great work here and I’m very happy to see it land upstream. Thanks!
This PR will merge in changes from vellottie -
It will also bring velato to a state where it can almost be published.
#14 will bring a followup to continue to prepare for release.