-
Notifications
You must be signed in to change notification settings - Fork 27
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
Slerp + dict of interpolation functions #62
Conversation
…cture than state, containing the corresponding interpolation functions. Added slerp. Also defaulted angles' method to slerp.
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.
Awesome PR @Fifourche - thanks so much for the time and effort!!
There are some things I like about this and some I like a bit less, I'd be really interested to hear your thoughts and what @sofroniewn and @guiwitz think about the following...
-
I like providing a custom interpolation function with the signature
f(a, b, fraction)
as you suggested more than providing separate 'forward' and 'inverse' transforms prior to standard interpolation as suggested by @sofroniewn in reintroduce slerp for camera interpolation #37. I think for more complicated transforms (like slerp and the relevant euler2quaternion stuff in this PR) it's easier to reason about and keep tidy... happy to be told I'm wrong here though! -
I'm not so sure on this being exposed as a keyword argument in the
Animation.capture_keyframe()
, I think it should be managed managed by us and invisible to the user. I can't imagine a situation where I want to manually set these to something different than the 'correct' function for a given attribute -
the cascading functionality is nice but I can't imagine a situation where I want one custom function for multiple attributes, did you have a specific use case in mind?
-
I think I prefer the notation @sofroniewn used to define attributes
'camera.zoom'
etc
{
'camera.zoom':{'forward': lambda x: np.power(10, x), 'inverse': np.log10)},
'camera.angles':{'forward': some_function, 'inverse': some_inverse_function},
}
Given what I've said above I think what we might want to do is
- create an
Interpolation
enum like we do for easing functions, in a new module calledinterpolation.py
, this should contain all current and future interpolation functions, moving them out ofutils.py
Easing func enum as an example is here
napari-animation/napari_animation/easing.py
Lines 242 to 289 in 829bc31
class Easing(Enum): | |
"""Easing: easing function to use for a transition. | |
Selects a preset easing function | |
* linear: linear interpolation between start and endpoint. | |
* quadratic: quadratic easing in and out. | |
Modeled after the piecewise quadratic | |
y = (1/2)((2x)^2) ; [0, 0.5) | |
y = -(1/2)((2x-1)*(2x-3) - 1) ; [0.5, 1] | |
* cubic: cubic easing in and out. | |
Modeled after the piecewise cubic | |
y = (1/2)((2x)^3) ; [0, 0.5) | |
y = (1/2)((2x-2)^3 + 2) ; [0.5, 1] | |
* quintic: quintic easing in and out. | |
Modeled after the piecewise quintic | |
y = (1/2)((2x)^5) ; [0, 0.5) | |
y = (1/2)((2x-2)^5 + 2) ; [0.5, 1] | |
* sine: sinusoidal easing in and out. | |
Modeled after half sine wave | |
y = 0.5 * (1 - cos(x * pi)) | |
* circular: circular easing in and out. | |
Modeled after the piecewise circular function | |
y = (1/2)(1 - sqrt(1 - 4x^2)) ; [0, 0.5) | |
y = (1/2)(sqrt(-(2x - 3)*(2x - 1)) + 1) ; [0.5, 1] | |
* exponential: exponential easing in and out. | |
Modeled after the piecewise exponential | |
y = (1/2)2^(10(2x - 1)) ; [0,0.5) | |
y = -(1/2)*2^(-10(2x - 1))) + 1 ; [0.5,1] | |
* elastic: elastic easing in and out. | |
Modeled after the piecewise exponentially-damped sine wave: | |
y = (1/2)*sin(13pi/2*(2*x))*pow(2, 10 * ((2*x) - 1)) ; [0, 0.5) | |
y = (1/2)*(sin(-13pi/2*((2x-1)+1))*pow(2,-10(2*x-1)) + 2) ; [0.5, 1] | |
* back: back easing in and out. | |
Modeled after the piecewise overshooting cubic function: | |
y = (1/2)*((2x)^3-(2x)*sin(2*x*pi)) ; [0, 0.5) | |
y = (1/2)*(1-((1-x)^3-(1-x)*sin((1-x)*pi))+1) ; [0.5, 1] | |
* bounce: bounce easing in and out. | |
""" | |
LINEAR = partial(linear_interpolation) | |
QUADRATIC = partial(quadratic_ease_in_out) | |
CUBIC = partial(cubic_ease_in_out) | |
QUINTIC = partial(quintic_ease_in_out) | |
SINE = partial(sine_ease_in_out) | |
CIRCULAR = partial(circular_ease_in_out) | |
EXPONENTIAL = partial(exponential_ease_in_out) | |
ELASTIC = partial(elastic_ease_in_out) | |
BACK = partial(back_ease_in_out) | |
BOUNCE = partial(bounce_ease_in_out) |
- create a dict of form
{
'camera.zoom' : Interpolation.LOG,
'camera.angles' : Interpolation.SLERP
}
for attributes which require a custom interpolation function
- add a mechanism for parsing that dict and using the specific functions on the specified attribute
We could also drop the enum and simple have a dict of functions - curious to hear your thoughts!!
I'm afraid I havn't got too much time right now to think deeply about this, but I like the idea of getting rid of my {
'camera.zoom' : Interpolation.LOG,
'camera.angles' : Interpolation.SLERP
} which seems quite elegant. I think this PR might also contain some changes from #61, so once we get that merged this should be easier to review. I should be able to get to it early next week, but in the mean time I say press on! 🚀 |
Thanks for your thoughts ! :) I agree with you, it should probably stay invisible to the user ! For this and for the cascading, what I had in mind at the time was the interpolation of the recently added layers' states. Some of the properties -not the base ones, but things like colormap or others- might be tricky to interpolate, and I imagined scenarios where the interpolation fucntion would need to change opacity as well as other parameters for instance. It was and still isn't clear in my mind how to prepare for this, but in any case I agree with everything you said above, and will adopt the proposed form ! Thanks again 😃 |
awesome - look forward to seeing what you come up with @Fifourche ! You should join us on the napari zulip chat - there are also developer meetings each week, it would be great to have you there! |
Codecov Report
@@ Coverage Diff @@
## main #62 +/- ##
==========================================
+ Coverage 62.70% 67.18% +4.48%
==========================================
Files 15 17 +2
Lines 732 835 +103
==========================================
+ Hits 459 561 +102
- Misses 273 274 +1
Continue to review full report at Codecov.
|
I made some changed according to the discussion ! I created a dictionary : interpolation_dict = {
"camera.angles": Interpolation.SLERP,
"camera.zoom": Interpolation.LOG,
} The unspecified properties will be defaulted to Also, I guess some tests should be changed accordingly ; I'll take care of that soon if you're ok with the commits so far ! |
… tests (removed the zoom specific test).
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.
Looking really great! I've left mostly minor, cosmetic comments, flagged a couple doc strings that need a few more sections, but otherwise this looks great and I think will then be good to merge @alisterburt when you're ready.
I'd also like to second @alisterburt comment about the napari zulip and communities meetings. I see you're in the zulip now, so I'll reach other there and give you additional details on the meetings.
napari_animation/interpolation.py
Outdated
# Dictionary relating state attribtues to interpolation functions | ||
interpolation_dict = { | ||
"camera.angles": Interpolation.SLERP, | ||
"camera.zoom": Interpolation.LOG, | ||
} |
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 wonder if we shouldn't move these to a different file/ locations, maybe put them directly onto our animation object. They could either be private, or I could imagine someone might want to overwrite them, add their own custom attributes to this dictionary and having them on Animation
seems like a good place.
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.
For now, I left it in interpolation.py
! I agree that we might want to define it as an Animation
attribute and pass it as a parameter to interpolate_state
, if there's no objection !
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.
@alisterburt what do you think? Agreed it's something that can come later, so we can merge this first and see
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.
Totally on board with having this as a public attribute of the Animation
- this seems like a great balance between the totally private current implementation and @Fifourche's more flexible first implementation. I'm not sure on the best name for this... state_interpolation_map
?
@Fifourche do you want to get this final change in before we merge this PR or would you rather we merge now and add it in a follow up PR?
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'm ok with state_interpolation_map
, and added it ! Should be ok now :)
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.
you're on fire! Will have a quick play then get to merging
…ngs and tests in consequence.
@Fifourche we've got some merge conflicts now since I merged #68, do you mind updating this PR. Then I think it is good to go!! |
…o custom_interpolation
…parameter to interpolate_state.
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.
Merge time 🚀
This PR is to propose a way for the user to give custom interpolation functions to compute frames. It also contains a Slerp default method. A related issue has been opened at #37 .
About custom interpolation functions
In details, I added a "methods" argument to the
capture_keyframe
function. "methods" is a dict of dicts containing functions, normally of same structure than "state".For instance, the custom interpolation function for the
state['camera']['angles']
would be looked for atmethods['camera']['angles']
.A few things to notice and to be discussed I think :
methods = {'camera': {'zoom': custom_function}}
. Other attributes' interpolation functions would be defaulted.custom_func(a, b, fraction)
methods = {'camera': custom_func}
.custom_func
will takeinitial_state['camera']
andfinal_state['camera']
dicts asa
andb
argumentsAbout Slerp
I took advantage of this to add a Slerp method, relying on
scipy
. The default interpolation function for "angles" is set to Slerp.An example code :
Here's an example code where "methods" is either set to a standard linear interpolation for "angles" and or defaulted, resulting in Slerp for "angles".
Playing with the slider will help visualizing the difference.