-
Notifications
You must be signed in to change notification settings - Fork 20
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
Keyframe reintroduction #48
Conversation
… branch. The Global Time Keeper node updates all keyframed properties using the fcurve feature. Not yet tested with the TimeSelector node. Code needs a bit of cleanup.
Hi Thomas, |
connected_converter_nodes = backtrack_converter_nodes(node) | ||
#Append, ignoring duplicates | ||
nodes_2b_updated = nodes_2b_updated.union(set(connected_converter_nodes)) | ||
|
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.
Need to add break here? Disabled Scene Time does not seem to work when frame is changed..?
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.
The problem comes probably from the ordering of the node tree. I will fix this by ignoring Time Selector nodes with active scene time directly in the AnimationHelper. Otherwise the Global Time Keeper can potentially overwrite the changes in these lines.
errors/bvtk_errors.py
Outdated
@@ -0,0 +1,18 @@ | |||
|
|||
class BVTKException(Exception): |
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.
Here comes now the question: Do we need a custom exception handler? Is it beneficial and justified, or could we use standard exception handlers?
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 found the custom handler helpful down the line to distinguish between expected and unexpected errors, but I'm not very attached to the idea. I'll leave this decision up to you.
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.
Well, maybe leave custom exception out now and add later on if needed. My aim is to try to make also error handling clearer in #45. Thanks!
time/animation.py
Outdated
current_frame = invalid_frame | ||
vtk_time = invalid_vtk_time | ||
def setup(self): | ||
self.current_frame = int(-1e6) |
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.
=invalid_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.
Will fix
time/animation.py
Outdated
self.current_frame = int(-1e6) | ||
self.animated_properties = None | ||
|
||
def get_animated_property_list(self): |
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.
Somewhat long and indented function, can it be split?
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.
Maybe I can extract common submethods with update_animated_properties to shorten the method
time/animation.py
Outdated
for f_curve in self.f_curves: | ||
prop_path = f_curve.data_path | ||
delimiter_index = prop_path.rindex(".") | ||
node_name = prop_path[7:delimiter_index-2] |
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.
Please add comment what is happening here
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.
Will fix
time/animation.py
Outdated
animated_properties = {k: (v[0], v[1], v[2], [tmp for tmp in zip(*v[3])], v[4], v[5]) for k, v in animated_properties.items()} | ||
return animated_properties | ||
|
||
def update_animated_properties(self, scene): |
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.
Similar code here and in get_animated_property_list(), maybe refactor common parts to own function possible?
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 will try to refactor the methods
customfilter.py
Outdated
return self.get_persistent_storage()["updated_nodes"] | ||
|
||
def setup(self): | ||
if self.bl_label in persistent_storage["nodes"]: |
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.
Add from .cache import persistent_storage needed?
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.
Will fix
time/animation.py
Outdated
@@ -0,0 +1,170 @@ | |||
''' |
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.
Lonesome file in own directory. Move to root and rename to animation_helper.py?
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.
Ok, I'll move it to the main directory as suggested.
OK I've added comments to code, please check them in github (I edited some comments). Overall this feature looks very promising! It also allows to keyframe Time Step value in Time Selector, so it should be possible to get arbitrary time point data from reader in, which I think was the idea in #44 . |
Many thanks for the thorough review and bug-checking. About the error handler, I'll await your decision and otherwise replace the exception thrown in assert_bvtk with a generic one. |
OK, so let's leave error handler out for now. |
- Invalid frame correctly referenced - Fixed a missing import in setup of Global Time Keeper - Removed the custom BVTKException and moved bvtk_assert to the core - animation.py moved to the main directory and renamed animation_helper.py - Fixed a bug where the Global Time Keeper could overwrite changes in Time Selector nodes with active 'Use Scene Time'
The new commits should resolve the issues and leave only the documentation to be done. |
Found few bugs:
|
The last two commits should hopefully fix the bugs you mentioned. |
Thank you so much! This feature addition is great! Everything I tried worked nice. |
Overview
This feature branch reintroduces keyframes from the standard Blender API back into the node tree. Every exposed property on any node should be animatable this way. This feature uses the fcurve feature along with the Global Time Keeper node to keep track and update the keyframes. It has been tested in limited ways, but looks to bring back a lot of animation functionality usable in other parts of blender. I'm looking forward on your thoughts.
Features
Preview
Limitations
Related Issues