Details

Reviewers

Sebastian Parborg (zeddb)
Demeter Dzadik (Mets)
Chris Clyne (lateasusual)
Julian Eisel (Severin)

Commits

rBc9c0f8930733: Action Constraint: Add manual time factor input control

Summary

Adds an optional slider to the action constraint so that it can be driven without a constraint target:

This is very helpful for more complex rigging and mechanical rigs, as it means the action constraint can be controlled with a driver/custom property directly, currently if we want to use a driver to control it we must add a "dummy" bone/object inbetween to act as a control.

Currently the input is a fixed "0-1" => "frame range" slider as that seemed like the most intuitive usage.

After reading the discussion on the other patch (https://developer.blender.org/D7572)
To be clear, this doesn't replace any existing functionality, only adds the "Use Factor" checkbox and the slider, and coincidentally is maybe more in line with what was suggested there in terms of UX.

@Sybren A. Stüvel (sybren) just did some final tweaks on the patch, original author is @Chris Clyne (lateasusual).

Diff Detail

Repository

rB Blender

Branch

arcpatch-D8022 (branched from master)

Build Status

Buildable 10211
Build 10211: arc lint + arc unit

Event Timeline

Chris Clyne (lateasusual) created this revision.Jun 14 2020, 5:49 PM

Harbormaster completed remote builds in B8539: Diff 25888.Jun 14 2020, 5:49 PM

Chris Clyne (lateasusual) requested review of this revision.Jun 14 2020, 5:49 PM

Chris Clyne (lateasusual) edited the summary of this revision. (Show Details)Jun 14 2020, 5:54 PM

Chris Clyne (lateasusual) added reviewers: Sybren A. Stüvel (sybren), Alexander Gavrilov (angavrilov).

Chris Clyne (lateasusual) added a project: Animation & Rigging.

This is a duplicate of D7572: ActionConstraint: Fallback Value. Though, I did a poor job of getting it accepted.

Dillon Gu (dillongoo) awarded a token.Jun 14 2020, 9:16 PM

Dillon Gu (dillongoo) added a subscriber: Dillon Gu (dillongoo).

In D8022#194482, @Wayde Moss (GuiltyGhost) wrote:

This is a duplicate of D7572: ActionConstraint: Fallback Value. Though, I did a poor job of getting it accepted.

Oof, I probably should have at least searched once first! I guess it's just one of those things lots of people want to see implemented ;)

I'll leave this up for now but it sounds like a devtalk thread is in order

Chris Clyne (lateasusual) edited the summary of this revision. (Show Details)Jun 14 2020, 9:32 PM

Wayde Moss (GuiltyGhost) awarded a token.Jun 14 2020, 10:40 PM

Demeter Dzadik (Mets) awarded a token.Jun 16 2020, 12:09 AM

Demeter Dzadik (Mets) added a subscriber: Demeter Dzadik (Mets).

I have the same issues with this patch as with D7572: the terminology is unclear. For one, it is mixing up "time" and "time_factor", which are two different things that should be expressed in different units. You can't just have "time" in C and "time_factor" in Python, and expect people to understand that this is the same thing. Then there are the terms "Time Factor" and "Factor", which apparently mean the same thing even though they use different terms. Please join forces with @Wayde Moss (GuiltyGhost) and get things designed properly.

This revision now requires changes to proceed.Jun 18 2020, 4:44 PM

Rename time -> time_factor, USE_TIME_INPUT->USE_TIME_FACTOR etc.

Also started a devtalk thread at https://devtalk.blender.org/t/action-constraint-slider-manual-input-property/13950 so we can decide on a better name / UI design if needed.

Harbormaster completed remote builds in B8629: Diff 26087.Jun 19 2020, 1:44 AM

Missed a couple of UI properties

Harbormaster completed remote builds in B8642: Diff 26115.Jun 19 2020, 5:55 PM

Rename time -> time_factor, USE_TIME_INPUT->USE_TIME_FACTOR etc.
Missed a couple of UI properties
Time Factor -> Evaluation Time, Fix accidental line swap
Rebase to current master

Harbormaster completed remote builds in B9019: Diff 26828.Jul 15 2020, 4:44 PM

Fix UI for drag/drop constraints refactor

I rebased onto master before i noticed the constraint drawing code had all been changed, new layout follows the same rules as the original.

Harbormaster completed remote builds in B9020: Diff 26830.Jul 15 2020, 5:00 PM

fix another accidental deletion

New Drag+Drop UI with slider:

Harbormaster completed remote builds in B9022: Diff 26832.Jul 15 2020, 5:15 PM

Sybren A. Stüvel (sybren) added a project: BF Blender (2.91).Sep 14 2020, 3:33 PM

Sybren A. Stüvel (sybren) edited reviewers, added: Sebastian Parborg (zeddb), Demeter Dzadik (Mets); removed: Alexander Gavrilov (angavrilov).

This looks good to me code wise and feature wise.

I've talked to the UI team (Julian Eisel) and the only nitpicks we have are:

The checkbox shouldn't remove items in this case. It should only grey out the unused/inactive items.
In addition to this, combine the checkbox and slider by using the new ui code in 2.90:
https://wiki.blender.org/wiki/Reference/Release_Notes/2.90/Python_API#User_Interface

A code example on how to do it is in the release notes link.

Move the Evaluation Time checkbox slider combo to under the Target object box.
So you will have:

Target
Evaluation Time
Mix

In the python UI def, drop the "Use" from Use Evaluation Time it it is only Evaluation Time.
The checkbox already implies that it is a "Use/Don't use" thing.
But keep the more descriptive text in the RNA definition (so don't drop use there).

Next is the tool tips:

Use Time input instead of a target object
Should be:
Use Evaluation Time input slider instead of a target object

I think.

The tool tip for the actual slider could be more descriptive as well:
Time of action within frame range
->
Determines which frame will be used in the defined frame range

That is all. After this is done, I think this is ready for master.

Implement UI Changes
Rebase onto current master

New UI with property greying etc.

Harbormaster completed remote builds in B10182: Diff 28844.Sep 14 2020, 7:05 PM

In D8022#217954, @Sebastian Parborg (zeddb) wrote:

Use Time input instead of a target object
Should be:
Use Evaluation Time input slider instead of a target object

I think.

With this tooltip in place, both the label and the tooltip read "Use Evaluation Time" (except that the tooltip of course continues the sentence). How about Interpolate between Action Start and End frame with the Evaluation Time slider, instead of the Target object/bone?

The tool tip for the actual slider could be more descriptive as well:
Time of action within frame range
->
Determines which frame will be used in the defined frame range

I think Interpolates between Action Start and End Frame is clearer, as it concretely states which frames are used, in the same terminology as the rest of the UI.

By using the term "interpolation" it, at least in my perception, becomes clearer how the number in the Evaluation Time slider relates to the frame start/end sliders.

Code wise the patch looks good to me. It could be polished a bit more by adding some comments.

source/blender/makesdna/DNA_constraint_types.h
341	Add a comment `/* Only used when flag ACTCON_USE_EVAL_TIME is set. */` to `eval_time`.
864	Add a comment `/* When set, ignore the transform of 'tar' and use eval_time instead. */`

I do like your tool tips better Sybren. So I would go with those descriptions instead! :)

The only thing I have to add now is that the "Target" box should be greyed out as well when the Evaluation Time checkbox is used.

I'm commandeering the revision to make the changes I mentioned myself. This way we can get it into master before bcon2 starts (which is tomorrow morning).