- 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 -
-
Looks good to me! Just one thing you could change if you feel like taking the time, but I don't think it's especially important.
Not critical, but if you want to take the time: this checks that keys weren't added for rotation and scale, but it doesn't check that a new key was added by autokey for the location channel, since the location channel already existed from manually creating a key.
Over-all looks good to me. Just some nits. I also haven't had a chance to test it yet, so still need to do that.
Maybe add a TODO here to investigate why that's the case? Because that seems pretty odd, and is perhaps a sign of something else that also needs some spring cleaning.
What is the integer return value? Let's document that too, because at least to me (having not seen the function body yet as I work my way through the PR) it's not at all obvious.
Really glad this note on frame parameters is here. However, what does NLA space mean in this case? Local time of the action? Time on the NLA track? I think this could be rephrased or expanded on to be clearer.
Looks good to me. I agree a rebuild shouldn't be needed here. And as you said even if a frame change does trigger something (e.g. via a handler) that would then require a rebuild, that itself should produce another signal that does trigger a rebuild.
The code looks good to me. Although maybe taking some time to add documentation comments at the top of the longer new functions.
As @Raymond-Luc pointed out to me, making a hotkey in the…
Ah, that's a good point. Yeah, splitting this up could be tricky.
The functional programmer in me wants to split out the idea of filtering fcurves (i.e. finding the fcurves that satisfy a given…
@ChrisLend In retrospect, I don't think changing the graph editor selection behavior is worth it. Certainly not in this PR, at least. If it's worth changing, we can revisit that in a future PR.
Some notes from testing:
- Over-all this is great!
- In Blender you always have an active object/bone, which is what determines what properties are displayed in the UI (e.g. in the n-panel). …
Over-all the code looks good to me. Just some nits, and a suggested refactor of one of the functions.