- 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 -
-
The code looks good to me! However, when I tested, it still doesn't completely match the old behavior: in 3.6 holding down ctrl has the described behavior even when snapping is otherwise disabled, whereas with this patch it does nothing. Everything else appears to be working perfectly, though.
Style: I would make this an else if. Or perhaps even better, make the whole thing a switch.
Looks like there are still some left over operator-based object adds.
Gah, I should have linked to what I found when I responded before. The change was made before I was part of the animation module. But I can at least give my own rationale of why I think it makes…
I'm not super familiar with the Python unit testing framework, but things basically look good to me. Just some minor comments.
I think it would be better to test for the exact frame values you expect here. Additionally, it might be good to add a key at a frame earlier than the action frame range to make sure that gets mapped back in properly as well.
I don't feel strongly about this, so feel free to ignore if your disagree. But since we're not testing the the add object operator, I would prefer to add objects via the Python API rather than by operator, and explicitly link it to the scene collection and make it selected+active. It's more verbose, but doesn't rely on implicit side effects of operators.
Similarly, I would prefer deleting the object via the Python API rather than by operator.
I might be misunderstanding something, but the list(set()) looks redundant to me. Can't it just be [fcurve.data_path for fcurve in fcurves] stand-alone?
#111143 addressed one aspect of things, but doesn't resolve the issue. The remaining parts of my proposal (in the comment further up) still haven't been done. Additionally, I think the proposal…
@PratikPB2123 I think so? I'm not aware of any code changes that would make this problematic to backport, at least.
Do you want me to take care of that, or should I leave that to you?
