Page MenuHome

Action Constraint: introduce a mix mode setting.
Needs ReviewPublic

Authored by Alexander Gavrilov (angavrilov) on Sun, Nov 24, 7:22 PM.

Details

Summary

Currently the action channels are applied after the existing
transformation, as if the action controlled a child of the
bone. This is not very natural, but more importantly, the
transform tools are not designed to work conveniently with an
additional 'pseudo-child' transformation, resulting in effects
like an unexpected pivot location.

Implementing a Before mode that integrates the action channels
as if applied to a parent allows using the special transform
tool code intended for dealing with such constraints.

Note that in either mode, Action constraints should be added
in reverse order, putting a new constraint before the existing
ones that the Action was keyframed to work together.

In order to implement the option, extract a utility from
the Copy Transform constraint code for combining transforms
with special anti-shear scale handling that matches the
Aligned Inherit Scale mode.

The Before mode also requires switching the constraint to
the Local owner space, while the After mode can still use the
World space for efficiency as before. Since the constraint
doesn't have an Owner space option in the UI, this has to be
handled in an RNA setter.

This change technically breaks backward compatibility, but
only when non-uniform scale is involved; the old behavior
will introduce shear in such cases, so I'm not sure strict
compatibility is worth an extra choice in the dropdown.

Diff Detail

Repository
rB Blender
Branch
temp-angavrilov-action-constraint (branched from master)
Build Status
Buildable 5967
Build 5967: arc lint + arc unit

Event Timeline

This is intended to supersede D6092 for T70805.

Test file - try rotating the bone with After and Before modes, and compare what happens with non-uniform scale in After mode with unpatched blender.

From a user perspective this seems good to me. I understand that After Original now avoids skewing when the parent is scaled non-uniformally, and how that technically breaks backwards comp. The reason I think that's fine is because I think as far as any user is concerned, the old behaviour felt unintentional to begin with, and we don't need to keep backwards comp with bugs. Also I'm pretty sure most people will want to use the new "Before Original" option, since it's more akin to how other (non-locking)constraints already work eg. Transformation, Copy Location with offset, etc. For the 0.01% that want After Original, the new scale inheritance is probably better than the old skewing one, although I don't have any strong feelings about this.

The other potential mixing options(with even fewer use cases) which we choose to omit here for the benefit of simplicity, can be covered in the far future by constraint nodes! :)

I don't have anything to add here. Looks good to me.

@Sybren A. Stüvel (sybren) does it look good to you too?

This revision is now accepted and ready to land.Mon, Dec 2, 10:33 AM

Oh, I guess you want to add Fix T70805: to the title?

Where is the broader design? This is something that's now being copied from one constraint to others, which to me indicates that there is a fundamental limitation of the constraint system. We should not keep copying workarounds for that limitation, but rather come up with a proper solution.

New features like this should also have automated tests. It shouldn't be too hard to construct a simple blend file that can be loaded, evaluated, and then checked for the world matrices. It is a very bad idea to add more complexity to an already complex system without automated tests to ensure nothing breaks.

Where is the broader design? This is something that's now being copied from one constraint to others, which to me indicates that there is a fundamental limitation of the constraint system. We should not keep copying workarounds for that limitation, but rather come up with a proper solution.

The 'proper solution' is nodes, I've mentioned this a great number of times.

New features like this should also have automated tests. It shouldn't be too hard to construct a simple blend file that can be loaded, evaluated, and then checked for the world matrices. It is a very bad idea to add more complexity to an already complex system without automated tests to ensure nothing breaks.

Nothing has 'automated tests' except cycles and BLI.

Considering the above, I don't feel that these remarks are relevant to addressing the specific practical issue at hand, solving which was requested by actual riggers.

This revision now requires changes to proceed.Tue, Dec 3, 2:43 PM

The 'proper solution' is nodes, I've mentioned this a great number of times.

I know, and in response I suggested that we should start working on a node-based constraint system.

Nothing has 'automated tests' except cycles and BLI.

Alembic, BMesh, GuardedAlloc, Collada, FFmpeg, parts of the keymap system, are all automatically tested. Sure, the tests have coverage far from 100%, but they are there and can be extended when necessary. The entire world uses automated testing to help in keeping complex systems maintainable, and Blender should do more automated testing too. I feel that especially in a complex system like the constraints, we really should start working on this.

Considering the above, I don't feel that these remarks are relevant to addressing the specific practical issue at hand, solving which was requested by actual riggers.

The current constraint system has worked for many years already, so it's not in a state that makes it impossible to use. Of course things can be improved, but at some point the limits of a system have been reached, and replacing it with something fresh and new that is designed for current demands is the way to go.

The current constraint system has worked for many years already, so it's not in a state that makes it impossible to use.

You know, thinking like that, why fix non-crash bugs, or in fact change anything at all - after all, people already manage to use blender somehow, so it's all ok, right?

As mentioned above, this is actually addressing T70805, and can be considered as a fix for a poorly though through design of this constraint (which can surely be also said about quite a few others too - e.g. scale handling in Spline IK earlier this year felt like nobody actually tried to use it in a real rig when implementing it; not to mention the insanity of using addition to combine scale in the Offset option of Copy Scale previously).

I'm happy to start brainstorming a design for a node-based replacement for the constraint system, but it would be a massive project, and there are no developers who can invest that time in the foreseeable future, so it would leave T70805 unfixed for the foreseeable future.

But, I guess there is a broader design, when it comes to these new constraint mix mode options. After all, they are easily definable, as a new feature that lets constraints combine their matrices in different ways. So maybe that is something that @Alexander Gavrilov (angavrilov) could do a quick write-up on? Do you think that would be useful @Sybren A. Stüvel (sybren) ?

can be considered as a fix for a poorly though through design of this constraint (which can surely be also said about quite a few others too - e.g. scale handling in Spline IK earlier this year felt like nobody actually tried to use it in a real rig when implementing it; not to mention the insanity of using addition to combine scale in the Offset option of Copy Scale previously).

And this is exactly why I'm pushing back against adding yet more options to yet more constraints. Especially when there is no concrete plan of which constraints will get those extra options, it makes me even less of a fan.

@Demeter Dzadik (Mets) we discussed yesterday that this constraint would be the last one we fix with when it comes to adding those Before/After Original options, so this change will go through.

@Alexander Gavrilov (angavrilov) Isn't there something we can do about maintaining backward compatibility? From what I understand, After Original is the original behaviour, and Before Original is what you added here. Can't we set Before Original as default, and use versioning code to set already-existing constraints to After Original?

And this is exactly why I'm pushing back against adding yet more options to yet more constraints.

I don't see how that's related. Actually, I attribute those original problems to "design"-based approach to implementation of the features, rather than based on specific needs arising during rigging, and without even properly testing them in a production-quality rig. Most of my patches arise from specific use cases/issues, although I often expand on the idea, like with all the mix modes.

Basically, with the mix modes, it started with the very practical problem that the Offset option of Copy Rotation was useless garbage, so I added some meaningful modes. Then I though that it would also make sense for other Copy-like constraints.

@Alexander Gavrilov (angavrilov) Isn't there something we can do about maintaining backward compatibility? From what I understand, After Original is the original behaviour, and Before Original is what you added here. Can't we set Before Original as default, and use versioning code to set already-existing constraints to After Original?

This doesn't change the default, it's still After in all cases. However, I changed the implementation to use the Aligned Inherit Scale like math to avoid creating shear because nobody actually wants shear, and do it in both modes for symmetry. The 'loss' of full backward compatibility is in that aspect. To maintain full compatibility it would either be necessary to leave shear in in the After mode, or add an extra "After (legacy)" mode, like with Inherit Scale "None (legacy)".

I.e. my logic here is that since nobody likes shear, animators and riggers would avoid using the constraints in situation when it would create it, and thus adjusting the behavior of the constraint just in those situations without full backward compatibility is probably ok.

I don't see how that's related. Actually, I attribute those original problems to "design"-based approach to implementation of the features, rather than based on specific needs arising during rigging, and without even properly testing them in a production-quality rig.

Ok, so it was based on limited design work.

Most of my patches arise from specific use cases/issues, although I often expand on the idea, like with all the mix modes.

That's good, and IMO that's what good design should do as well.

Basically, with the mix modes, it started with the very practical problem that the Offset option of Copy Rotation was useless garbage, so I added some meaningful modes. Then I though that it would also make sense for other Copy-like constraints.

What I'm suggesting is exactly what you've done, but then with a bit more discussion beforehand about possible solutions to the problem, and some more planning of this work up front. Having a discussion and getting more people involved in a project will make the design better; different people see different things, and from what you describe that's exactly what was lacking in the old constraint design.

Often problems in the code are simply the result of someone working alone for too long, and with all the good intentions in the world doing what she/he thinks is right. Doing things alone can cause people to miss an outside perspective, miss certain important use cases, and thus end up with a limited system. I think this is what happened in the past, and I want to avoid that in the present.

This doesn't change the default, it's still After in all cases. However, I changed the implementation to use the Aligned Inherit Scale like math to avoid creating shear because nobody actually wants shear, and do it in both modes for symmetry. The 'loss' of full backward compatibility is in that aspect. To maintain full compatibility it would either be necessary to leave shear in in the After mode, or add an extra "After (legacy)" mode, like with Inherit Scale "None (legacy)".
I.e. my logic here is that since nobody likes shear, animators and riggers would avoid using the constraints in situation when it would create it, and thus adjusting the behavior of the constraint just in those situations without full backward compatibility is probably ok.

You can only say "nobody likes shear" after you've asked everybody. There are so many different ways in which people use Blender that for me it's hard to know how many people's blend files actually depend on the current behaviour. We're not just talking about character riggers here, but all users of the constraint. If we can keep things backward compatible without too much of a hassle, let's do that.

As to naming the backward-compatible option, I'm never fan of labels like "legacy", "old", or "new". Often they don't convey any meaning as to the behaviour of the feature. They split the timeline into two parts ("legacy" and "current"), which means that to understand the terminology you have to know when that split took place, and how Blender behaved before it.

As to naming the backward-compatible option, I'm never fan of labels like "legacy", "old", or "new". Often they don't convey any meaning as to the behaviour of the feature. They split the timeline into two parts ("legacy" and "current"), which means that to understand the terminology you have to know when that split took place, and how Blender behaved before it.

In my view, "legacy" means "this is here only for backward compatibility, don't use anymore". Basically, a milder synonym of "obsolete".

So, in practical terms, are you suggesting adding the third option?

As to naming the backward-compatible option, I'm never fan of labels like "legacy", "old", or "new". Often they don't convey any meaning as to the behaviour of the feature. They split the timeline into two parts ("legacy" and "current"), which means that to understand the terminology you have to know when that split took place, and how Blender behaved before it.

In my view, "legacy" means "this is here only for backward compatibility, don't use anymore". Basically, a milder synonym of "obsolete".

I think that's very good to mention in the tooltip, but to me it is an indicator of the status of the functionality, and not an indicator of the functionality itself. IMO the label should try and describe the behaviour.

So, in practical terms, are you suggesting adding the third option?

Yes, that way we can maintain backward compatibility. I wish we had telemetry on which features of Blender are (not) used so we could make a more informed decision, but barring that I think we should strive for backward compatibility when possible.

I guess the "Additive" checkbox on the Copy Scale constraint might be a good reference here. "Additive" is descriptive, and the tooltip says that it's for 2.7 compatibility.

Added the fully backward compatible option.