Seperating the Timeline into Timeline and Animation by m-agour · Pull Request #694 · fury-gl/fury

m-agour · 2022-09-26T19:24:20Z

In this PR, a new structure for the animation module is introduced while maintaining the old API (except for add_child_timeline is changed to become add_child_animation)
There is still modifing the tests. Tutorials will be updated in another PR.

skoudoro

See below a quick review, I will look deeper tomorrow

skoudoro · 2022-09-27T01:37:19Z

+    def _get_data(self, is_camera=False):
+        if is_camera:
+            self._is_camera_animated = True
+            return self._camera_data
+        else:
+            return self._data


you split it for all the other functions so you should split here also (_get_data and get_camera_data)

skoudoro · 2022-09-27T01:38:29Z

+import numpy as np
+from scipy.spatial import transform


import order, check pep

skoudoro · 2022-09-27T01:46:38Z

+                colors = [1, 1, 1]
+        if len(lines) > 0:
+            lines = np.array([lines])
+            if colors is []:


avoid is, it will return what you expect. use isonly with None otherwise to check copy. see below

>>> a = [] >>> a is [] False

skoudoro · 2022-09-27T01:48:03Z

+    def get_keyframes(self, attrib=None, is_camera=False):
+        """Set a keyframe for a certain attribute.


Get or Set ?

skoudoro · 2022-09-27T01:49:10Z

+        is_camera: bool, optional
+            Indicated whether setting a camera property or general property.
+        update_interpolator: bool, optional
+            Interpolator will be reinitialized if Ture


skoudoro · 2022-09-27T01:55:57Z

+            function that does not depend on keyframes.
+
+        Examples
+        ---------


skoudoro · 2022-09-27T01:56:33Z

+            Specifies whether the `interpolator` is time-only based evaluation
+            function that does not depend on keyframes.
+        Examples
+        ---------


too long and miss empty line between 2 sections

skoudoro · 2022-09-27T01:56:52Z

+        Examples
+        ---------


same as above

skoudoro · 2022-09-27T01:57:01Z

+        Examples
+        ---------


same as above

skoudoro · 2022-09-27T02:01:44Z

+        attrib = self._get_attribute_data(property_name)
+        attrib.get('callbacks', []).append(cbk_func)
+
+    def update_animation(self, t=None):


avoid 1 letter variable. 3 letters minimum. to update

skoudoro · 2022-09-27T14:13:14Z

All tests are failing, can you fix the issue in this PR? see here: https://github.com/fury-gl/fury/actions/runs/3135809301/jobs/5091933852#step:12:433

this is due to the renaming of add_child_timeline

…ine`-into-`Timeline`-and-`Animation`

m-agour · 2022-09-27T15:01:04Z

All tests are failing, can you fix the issue in this PR? see here: https://github.com/fury-gl/fury/actions/runs/3135809301/jobs/5091933852#step:12:433

this is due to the renaming of add_child_timeline

@skoudoro Should I change the Timelines into Animations as well, in glTF animation?

skoudoro · 2022-09-27T19:01:14Z

@skoudoro Should I change the Timelines into Animations as well, in glTF animation?

No @xtanion will have to do that, it will avoid conflict for him.

Thank you for the update

filipinascimento

Thanks, this one looks good. Please check other places that you still may be calling timeline.add_child_animation.

filipinascimento · 2022-10-03T13:40:03Z

+
+###############################################################################
+# Adding the main animation to the Timeline.
+timeline.add_child_animation(main_arm_animation)


This should be timeline.add_animation, correct?

No matter if it was set automatically of by a Timeline

…ine`-into-`Timeline`-and-`Animation`

codecov · 2022-11-08T20:58:36Z

Codecov Report

Merging #694 (207853e) into master (8e1cc27) will increase coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head 207853e differs from pull request most recent head 31758d9. Consider uploading reports for the commit 31758d9 to get more accurate results

@@            Coverage Diff             @@
##           master     #694      +/-   ##
==========================================
+ Coverage   50.19%   50.21%   +0.02%     
==========================================
  Files         120      126       +6     
  Lines       28019    28291     +272     
  Branches     2987     3033      +46     
==========================================
+ Hits        14063    14206     +143     
- Misses      13489    13611     +122     
- Partials      467      474       +7

Impacted Files	Coverage Δ
fury/animation/__init__.py	`0.00% <0.00%> (ø)`
fury/animation/animation.py	`0.00% <0.00%> (ø)`
fury/animation/tests/test_animation.py	`0.00% <0.00%> (ø)`
fury/animation/tests/test_timeline.py	`0.00% <0.00%> (ø)`
fury/animation/timeline.py	`0.00% <0.00%> (ø)`
fury/gltf.py	`0.00% <0.00%> (ø)`
fury/fury/gltf.py	`80.33% <0.00%> (ø)`
fury/fury/animation/__init__.py	`100.00% <0.00%> (ø)`
fury/fury/animation/tests/test_animation.py	`98.79% <0.00%> (ø)`
fury/fury/animation/animation.py	`69.15% <0.00%> (ø)`
... and 5 more

Garyfallidis · 2022-11-24T03:56:13Z

@m-agour please rebase.

…ine`-into-`Timeline`-and-`Animation`

m-agour · 2022-12-02T01:41:03Z

Hi @Garyfallidis @skoudoro, The scipy problem has been fixed now.

Garyfallidis · 2022-12-02T03:05:11Z

Thank you @m-agour.

m-agour added 12 commits September 25, 2022 16:12

Seperating Timeline into Timeline and Animation

c219c40

Modifing tests

a97754a

Fixed some docs

49d0d73

Added more docs

03c14d2

Duration should be initialized

e158720

Animation playable on its own

2105548

parent animation impl and doc

fb9df0d

updated robot arm tutorial

af350a1

pep fixing

c4a0c6d

Tutorials adapt to new method

83e716c

Update duration in case of hard setting the length

f255501

Set back playback_panel to False untill tutorials are edited

7d5615b

skoudoro added the type:Enhancement label Sep 26, 2022

skoudoro reviewed Sep 27, 2022

View reviewed changes

m-agour added 5 commits September 27, 2022 14:39

@skoudoro review - pep and doc issues

0d602f4

Impl get_camera_data method

373268e

get_keyframes doc typo

965daa1

Too long dash fixed

bf1ab65

Fixed one letter param

fc67551

Merge remote-tracking branch 'upstream/master' into Seperating-`Timel…

5a8c805

…ine`-into-`Timeline`-and-`Animation`

fixed glTF animation test

097cb3f

m-agour added 6 commits September 28, 2022 11:21

Optimized motion path and some docs

cc806e4

assert actor is not already added to animation

8b35e9a

updated tests

2682f5b

sleep time is not accurate and fails the tests on some OSs

426936b

Added loop property

5c23ece

making Timeline independent

117c270

m-agour added 2 commits October 1, 2022 20:23

updated tests

9751457

Adding animations after adding creating the PlaybackPanel

a2f8030

filipinascimento reviewed Oct 3, 2022

View reviewed changes

m-agour added 18 commits October 4, 2022 12:36

some renaming and loop issue fixed

c45dc5b

Abandoned the Container as a superclass

37ab99c

Separated camera animation from normal animation

87d2169

Updated docs

0a73f7e

Added 'camera' property and some docs

1d0c673

Added tests for CameraAnimation

1c32b2d

renaming update_timeline to just update

0617d87

Animation loop adjustment

cee5723

Added the ability to get current timestamp of Animation

0fb1880

No matter if it was set automatically of by a Timeline

Added support for general callbacks

331e755

Merge remote-tracking branch 'upstream/master' into Seperating-`Timel…

bddb34a

…ine`-into-`Timeline`-and-`Animation`

Merge remote-tracking branch 'upstream/master' into test2!

a8e9d6b

Altered current_timestamp of single animations

12716f6

Migrated glTF anim to the new animation sys

599ec5a

Renamed some timelines leftovers

8e7092b

Updated glTF animation tests

e409e2b

Let them loop

14897c0

updated glTF tutorials

207853e

m-agour changed the title ~~Seperating Timeline into Timeline and Animation~~ Seperating the Timeline into Timeline and Animation Nov 8, 2022

m-agour added 2 commits November 26, 2022 10:32

Merge remote-tracking branch 'upstream/master' into Seperating-`Timel…

0d11f81

…ine`-into-`Timeline`-and-`Animation`

Test scipy import fix

31758d9

Garyfallidis merged commit 8710a65 into fury-gl:master Dec 2, 2022

skoudoro mentioned this pull request Dec 2, 2022

Add Timelines to ShowManager directly #690

Merged

		def get_keyframes(self, attrib=None, is_camera=False):
		"""Set a keyframe for a certain attribute.

Conversation

m-agour commented Sep 26, 2022

Uh oh!

skoudoro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skoudoro commented Sep 27, 2022

Uh oh!

m-agour commented Sep 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skoudoro commented Sep 27, 2022

Uh oh!

filipinascimento left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Garyfallidis commented Nov 24, 2022

Uh oh!

m-agour commented Dec 2, 2022

Uh oh!

Garyfallidis commented Dec 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

m-agour commented Sep 27, 2022 •

edited

Loading

codecov Bot commented Nov 8, 2022 •

edited

Loading