Page MenuHome

Add Custom Space to Constraints
Needs ReviewPublic

Authored by Henrik Dick (weasel) on Apr 15 2020, 5:44 PM.

Details

Summary

Constraints can currently only use World Space, Local Space, Pose Space, Local with Parent.
This patch adds Custom Space with a custom object to define the space.

The motivation for this patch is the following situation.
If you have a copy location constraint in your armature you are already limited when you don't want to use a specific axis. It can be done, but its unneccessarily difficult (as far as I know). But the situation is close to impossible as soon as you have a root bone in your armature (which I was told you always want). I need to only copy the location along a custom axis for a lot of things. The same goes for copy rotation/scale and all the limit constraints because I need to make that axis relative to the root bone transform.

Here is a demo video:

This is the file of that video:

Actually this is the limiting factor for most of the things that I did so far with bones.

Diff Detail

Repository
rB Blender

Event Timeline

Henrik Dick (weasel) requested review of this revision.Apr 15 2020, 5:44 PM
Henrik Dick (weasel) created this revision.

I started this because I need this badly! I was always working around this until I realized today that this is what I really need.

Currently it still crashes everytime I open a file saved with some constraint with object mode.

I uploaded this diff this early because I need feedback before I commit more time to it.
Also I kind of don't know yet, what things I am influencing with my changes and how much it is going to break the way I added it. I really need some advise to push that further.

Also I don't know who is the right person to take a look at this so I hope somebody will assign the right reviewers for me.

"Object Space" in standard CG terminology means the same as "Local Space". So this will need a different name, maybe "Custom" something.

But mainly what this patch lacks is a motivation for why this feature is needed, with examples.

Henrik Dick (weasel) edited the summary of this revision. (Show Details)Apr 15 2020, 6:43 PM

@Brecht Van Lommel (brecht) for the name I would argue that object space would go hand in hand with object texture coordinates. For texture coordinates we have local, global and object space.

  • cleanup and simplification
  • file loading now works (I needed the id loopers)

This is my test file with some examples of things that would have been hard or impossible before

@Brecht Van Lommel (brecht) for the name I would argue that object space would go hand in hand with object texture coordinates. For texture coordinates we have local, global and object space.

It seems to be called that way in the Displace modifier, inconsistent with the shader nodes. I guess constraints can call it Object as well and if we ever unify the naming we can change it.

  • further cleanup
  • fixed some updating issues
  • fixed space conversion for bones

I think this is actually ready for review now.

Here is my updated demo file

Naming wise, I do share the confusion that "Object Space" is then all of a sudden not "the local space of this object". These constraints are highly mathematical; let's not use names that confuse people who are familiar with the math. "Object Space" also doesn't confer whether it's that object's local or world space, but this could be something that's explained in the tooltip + manual. Maybe "Custom Object" would be better, that at least to me is more clear in that it is neither the constrained object itself nor the target object.

source/blender/blenkernel/intern/constraint.c
639

I'm guessing obmat is the matrix of the object. This should get a new name, as it's understandable within the context of this patch, but otherwise it's too vague. Something like space_obj_world_matrix would be better IMO, as it also immediately conveys whether it's a local or a world matrix.

920–926

Coding style wise, I would negate the condition and do an early return:

if (!con || !targets ||
    (con->ownspace 1= CONSTRAINT_SPACE_OBJECT && con->tarspace != CONSTRAINT_SPACE_OBJECT)) {
  return 0;
}

bConstraintTarget *ct;
SINGLETARGET_GET_TARS(con, con->space_obj, con->space_subtarget, ct, targets);
return 1;

That way there is a clear separation between precondition checks and actual functionality of the function.

source/blender/makesdna/DNA_constraint_types.h
67

Same comment as above, this should be renamed to avoid confusion.

source/blender/makesdna/DNA_constraint_types.h
67

Does runtime data need to be stored here at all? Storing it in DNA should be avoided whenever possible.

@Brecht Van Lommel (brecht) The idea with storing it in the DNA was that other function like the one in armature add could access it, but I do also think that its quite unclean. How about adding a getter function to get the object space if needed from anywhere? I think that may be a lot cleaner. Will try to do that and see what you think.

Henrik Dick (weasel) marked 4 inline comments as done.
  • rename to Custom Object Space
  • moved evaluation data from DNA to ConstraintOb which is meant for evaluation data
  • moved getting the space matrix to a seperate function that is accessible from everywhere
  • made requested/proposed changes
Sybren A. Stüvel (sybren) requested changes to this revision.Apr 20 2020, 5:32 PM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/blenkernel/intern/constraint.c
928

flush_space_tar should adhere to this signature:

void (*flush_constraint_targets)(struct bConstraint *con, struct ListBase *list, bool no_copy);

In other words, it should return void and not int.

This revision now requires changes to proceed.Apr 20 2020, 5:32 PM
  • rebase
  • fixed the accidental wrong function signature (due to Ctrl+C, Ctrl+V... bad habit)

@Sybren A. Stüvel (sybren) feedback seems to be addressed. Did you check with out animation/rigging team this is good addition?

I read the description and opened the updated example file... but I'm honestly still a bit confused about what exactly is being talked about here. In the example file the text reads "Rotate the stuff that has Emties (not the Emties themselves" and I'm not sure what "stuff" is being referenced, nor how it does something wrong or right. Perhaps I haven't had enough coffee this morning.

This might be a great example of something that could be better explained with a short video demonstration. Is there any chance we could get something like that?

Henrik Dick (weasel) edited the summary of this revision. (Show Details)Apr 28 2020, 1:03 PM
Wo!262 (wo262) added a subscriber: Wo!262 (wo262).

Could another bone, or another bone in another object be used? Then calling it custom would make sense.

Yes even vertex groups can be used. So I will rename it to Custom Space (currently it is Custom Object Space)

Looks super interesting to me, I can see the usefulness in specific cases. I'd be interested in hearing @Demeter Dzadik (Mets)'s thoughts on this.

Henrik Dick (weasel) retitled this revision from Add Object Space to Constraints to Add Custom Space to Constraints.
Henrik Dick (weasel) edited the summary of this revision. (Show Details)
  • rebase
  • rename to Custom Space in the UI (everything else was already renamed)

To me this sounds and looks useful, although I don't rig mechanical things a lot, I can see the use cases there. But can possibly even find a use in character rigging.

I have a couple of notes, but note that I tested before the last update:

  • The new space option only seems available when the constraint's target is an Armature. This made the cubes with the wave modifier extremely confusing to understand what's going on, since the custom target options weren't visible.

  • I think this makes the "Use Rotation" checkbox for the Floor constraint redundant, but I'm not 100% sure. Something worth checking. At a short glance at least, enabling that checkbox seems the same as setting the spaces to the new option.

Yes the problem why the object selection text box did not appear when a object is selected was a typo, fixed with the last update

Looks interesting!

Just one question, would it be possible to control this object that you are using from within the rig, or would it cause some kind of deps graph dependency loop?

I mean, what if we parent a cube to one of the bones of the armature and then try to use that cube as the coordinate space in one of the bone constraints of the armature?

Having said that.... Would it be possible to extend this functionality to even use a bone as the coordinate space???????????? Just thinking out loud :)

@Sybren A. Stüvel (sybren) can we get this patch ready to land and commit before the constraint panel gets reworked? Currently it still applies cleanly onto master (for me at least). Afaict by the comments, people seem to love it and want it.

Henrik Dick (weasel) planned changes to this revision.Fri, Jun 19, 8:11 PM

Well now since the constraint UI got changed I need to do a fairly large rebase...

  • Rebase after new constraint ui list

It was much less work than I anticipated to rebase the patch. Anyway, when will this patch get any attention again? As far as I can see it, it was finished a long time ago already (or at least all comments had been adressed)

  • adjusted spacing of the subtarget selector in the constraint panel

I think your best bet is to poke some devs on blender.chat or elsewhere on a weekday, but patience is also key :) they are busy bees!

Sybren A. Stüvel (sybren) requested changes to this revision.Fri, Jun 26, 11:14 AM

@Henrik Dick (weasel) Apart from the few nags the patch looks good to me. For future patches, please use Arcanist to submit the patch. That way Phabricator knows which commit of which repository it is based on, and thus it'll show more context here, making it easier to do the review.

The only thing that's still missing are unit tests. Do you think you could add some tests for the new space conversion to tests/python/bl_constraints.py?

release/scripts/startup/bl_ui/properties_constraint.py
250

No need to separate with an extra newline (here and the the changes below as well).

source/blender/blenkernel/BKE_constraint.h
217

Follow the proper name style for return parameters.

source/blender/blenkernel/intern/fcurve_driver.c
406

Use NULL instead of 0. Same for other places.

source/blender/makesdna/DNA_constraint_types.h
65

I would prefer to not shorten this, and just name the field space_object. This would also use the same name in RNA and DNA.

68–71

Not sure why this was moved down. I think it's better to put the new properties directly under the tarspace declaration.

source/blender/makesrna/intern/rna_constraint.c
207

If you change this to "relative to a custom object/bone/vertex group" it's clearer what the "Custom" refers to in the name. Same below.

source/blender/makesrna/intern/rna_object_api.c
332

This message doesn't explain why the 'to_space' is invalid. Note that the others all do do this:

"'from_space' '%s' is invalid when no pose bone is given!"
"'to_space' '%s' is invalid when no pose bone is given!"

From a usability view it's important to explain to users what they're doing wrong.

Looking at the code, I don't even know why this would be wrong. If the "to" space should never be "custom", it shouldn't be selectable in the first place. Is this a scenario in which users can select something that'll never be supported? Or is this not something users can select, and is it just some extra security should a value somehow be changed to an invalid value?

This revision now requires changes to proceed.Fri, Jun 26, 11:14 AM
Henrik Dick (weasel) marked 7 inline comments as done.
  • make requested changes

I will try to add some unit tests, but I have never done that before, so it might take a few hours.

source/blender/makesdna/DNA_constraint_types.h
68–71

I did this because Hans Goudey used the padding for ui_expand_flag and therefore things (especially the object) missalign to the 4 byte grid and I get DNA compilation errors. At least for the last time I checked. You can change this for the final commit If you want to use padding.

source/blender/makesrna/intern/rna_object_api.c
332

This is extra security if something gets changed in the future. Users are not able to select this afaik.

  • add simple tests for constraint space

Here is the updated blend file for the tests (I didn't know how to include it in any other way)

Henrik Dick (weasel) edited the summary of this revision. (Show Details)Sat, Jun 27, 1:18 PM

Since the latest revision renamed a DNA entry the files created with this revision needed an update, so here is the new file that was used for manual testing and demonstration purposes.

  • added missing id_looper, flush and get functions to the distance constraint.