Keyframe animation with camera support #626
Conversation
|
Hello @m-agour! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-07-30 18:00:42 UTC |
There was a problem hiding this comment.
Hi @m-agour,
Thank you for this PR.
See below for some basic comments.
Overall, this is missing unit tests (as you mentioned). Also, I wonder if the interpolator should be function-based instead of class-based. All the functions can be independent and I do not see the value of all this inheritance apart from complexifying the codebase. I will be happy to discuss it.
Concerning your use of a dictionary, I recommend using more dict.get instead of dict[] to avoid strange errors when a key is missing.
The last point is to increase your use of propertydecorator to be more pythonic.
I would recommend focusing on this PR in the next few days. It will be great if it can follow the standard and get merged. After that, we could go to more advanced features.
Thank you.
fury/colormap.py
Outdated
| @@ -623,3 +624,335 @@ def hex_to_rgb(color): | |||
| b = int("0x" + color[4: 6], 0) / 255 | |||
|
|
|||
| return(np.array([r, g, b])) | |||
|
|
|||
|
|
|||
| # Implementation of this function is taken from scikit-image package. | |||
There was a problem hiding this comment.
Not the right place. you should add a note section in your docsting:
Notes
------
Original Implementation from scikit-image package. it can be found at https://....
this implementation might have been modified.
fury/colormap.py
Outdated
| return out | ||
|
|
||
|
|
||
| # Implementation of this function is taken from scikit-image package. |
There was a problem hiding this comment.
Not the right place. you should add a note section in your docsting:
Notes
------
Original Implementation from scikit-image package. it can be found at https://....
this implementation might have been modified.
fury/colormap.py
Outdated
| rgb_from_xyz = linalg.inv(xyz_from_rgb) | ||
|
|
||
|
|
||
| # Implementation of this function is taken from scikit-image package. |
There was a problem hiding this comment.
Not the right place. you should add a note section in your docsting:
Notes
------
Original Implementation from scikit-image package. it can be found at https://....
this implementation might have been modified.
fury/colormap.py
Outdated
| return arr | ||
|
|
||
|
|
||
| # Implementation of this function is taken from scikit-image package. |
There was a problem hiding this comment.
Not the right place. you should add a note section in your docsting:
Notes
------
Original Implementation from scikit-image package. it can be found at https://....
this implementation might have been modified.
fury/colormap.py
Outdated
| 'R': (1.0, 1.0, 1.0)}} | ||
|
|
||
|
|
||
| # Implementation of this function is taken from scikit-image package. |
There was a problem hiding this comment.
Not the right place. you should add a note section in your docsting:
Notes
------
Original Implementation from scikit-image package. it can be found at https://....
this implementation might have been modified.
fury/animation.py
Outdated
| from fury.actor import Container | ||
| from fury.colormap import rgb2hsv, hsv2rgb, rgb2lab, lab2rgb, xyz2rgb, rgb2xyz | ||
| from fury.ui.elements import PlaybackPanel | ||
| from vtkmodules.vtkRenderingCore import vtkActor |
There was a problem hiding this comment.
from fury.lib import Actor instead
fury/animation.py
Outdated
| @staticmethod | ||
| def _lerp(v1, v2, t1, t2, t): | ||
| if t1 == t2: | ||
| return v1 | ||
| v = v2 - v1 | ||
| dt = (t - t1) / (t2 - t1) | ||
| return dt * v + v1 | ||
|
|
||
| @staticmethod | ||
| def _get_time_delta(t, t1, t2): | ||
| return 0 if t <= t1 else 1 if t >= t2 else (t - t1) / (t2 - t1) |
There was a problem hiding this comment.
staticmethod and protected? strange. can you explain?
fury/animation.py
Outdated
| from vtkmodules.vtkRenderingCore import vtkActor | ||
|
|
||
|
|
||
| class Interpolator(object): |
There was a problem hiding this comment.
no need of object since python 3.
fury/animation.py
Outdated
| math.sqrt((x[1] - y[1]) * (x[1] - y[1]) + | ||
| (x[0] - y[0]) * (x[0] - y[0]))) | ||
|
|
||
| def get_points(self): |
There was a problem hiding this comment.
Why this function is only for this interpolator?
There was a problem hiding this comment.
I needed the points to calculate the linear lengths only for spline interpolator.
Yeah, it was being used once, and it was simple, so I just added the implementation of it to the setup method directly.
fury/animation.py
Outdated
| self.id = 5 | ||
|
|
||
|
|
||
| class Timeline(Container): |
There was a problem hiding this comment.
To be more pythonic, you need to use more the property decorator.
Hi @skoudoro, thank you so much for your review. |
Still needs some work
Codecov Report
@@ Coverage Diff @@
## master #626 +/- ##
==========================================
- Coverage 52.04% 50.45% -1.60%
==========================================
Files 108 113 +5
Lines 23812 25823 +2011
Branches 2631 2919 +288
==========================================
+ Hits 12394 13028 +634
- Misses 11013 12341 +1328
- Partials 405 454 +49
|
|
superseded by #647. Closing |
This is the no-shader version of the animation system that consists of:
TimelinePlaybackPanelI also included some tutorials. They are located at
tutorials/01_introductoryand their file name starts withviz_keyframe_animation.Docs are not complete. Will write them and the tests along this week.
Below is a record of tutorial
viz_keyframe_animation_camera.pyslerp.2022-07-13.15-15-49.mp4