- 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 -
-
Since rctf is a fairly small plain-old-data struct, I think it would be clearer to simply return it rather than using an output parameter. And then the callee can union it with another rctf if desired, rather than that being an implicit part of this function.
The name of this function led me to believe that it was side-effect free, but in fact it's unhiding the fcurves as well. So I'm wondering if it makes sense to split this out into functions that serve as clearer vocabulary. For example:
Nit: out-of-context I don't think isolate is very descriptive for the second argument. It's not actually isolating something, rather it's hiding everything. So maybe just hide?
I've tested this out, and everything works as expected. The code also looks about as good to me as I think can be done given the broader snapping code it's working within.
I'm unable to reproduce locally, at least not with the steps provided in the issue description. However, I'm on Linux with an AMD graphics card, so maybe this is hardware/driver specific?
@Nurb…
@mano-wii This patch is about fixing a regression in behavior, rather than improving the feature. We can look into improving behavior in 4.1.
I would argue this was never an intended feature
Yeah, that doesn't surprise me, honestly. Still good to match the old behavior first, though, I think. Then we can figure out where to go…
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.
Similarly, I would prefer deleting the object via the Python API rather than by operator.