- Amsterdam, Netherlands
- https://cessen.com
-
Animator, rigger, and software developer. Currently working at the Blender Institute as a developer on Blender's animation system.
Been using Blender since 1998, and worked on Big Buck Bunny and Sintel (two of Blender's open movie projects).
- Joined on
2003-03-21 -
-
👍
It looks like you forgot to delete this (now duplicate) version of rna_id_animdata_fix_paths_rename_all() that these comments are still referencing, though?
Oooh, wait, it looks like the version of this function on ID is still left over (after you added it to AnimData). The version on AnimData is correct.
Not sure if I accidentally marked this as resolved or if you did, but it looks like you haven't changed MAX_ID_NAME to MAX_IDPROP_NAME yet, so unresolving.
(No big deal or anything, just…
It looks to me like MAX_IDPROP_NAME is probably the right one to use here.
Re: FUNC_USE_SELF_ID
I'm not 100% sure (RNA is still something I'm learning). But poking around for other examples I found rna_Driver_new() and rna_Driver_remove() in rna_animation.cc.…
Just one last thing that's not clear to me. Works great in testing, though.
Is this min() necessary? Also the max() in the if (right_outside_key_index >= 0) below.
Let's just leave it as-is for now since it was already this way before this PR, and that minimizes chances of accidental behavior changes. A cleanup PR after this would be nice if you want to…
Ooooh, got it. That was a total brain fart on my part. Disregard this comment. For some reason I was imagining the fcurve parameter as being some kind of path-based descriptor... even though…
This just something to consider, not a "please change this" comment.
Do I understand correctly, then, that (at least for now) this function is specific to Baklava phase 1? I.e. it won't work properly for multi-layer/multi-strip actions?
Ah, got it. Yeah, that would be pretty awkward. Maybe there's a common pattern to handle that in modern c++? Dunno, though. I think for now the callback approach is probably nicer, and we can…
The code generally looks good to me. However, while testing I realized that this doesn't fully solve the issue because there's another case this doesn't account for: the left and right keyframe of a span both being off screen on opposite sides. E.g. if the user zooms way in so the left keyframe is off the left side of the screen and the right keyframe is off the right side of the screen.