Anim: Change how names of Bindings work, and how Bindings are created/assigned #120941
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#120941
Loading…
Reference in New Issue
No description provided.
Delete Branch "dr.sybren/blender:anim/baklava-binding-name-strip-2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This cleans up some of the Animation/Binding API, and adds a distinction between a binding's "name" and its "display name".
An example file is attached below.
name
Animation
. As such, it is also the key into theanim.bindings
collection.name
is always prefxed with the ID identifier, likeOBCube
andCACamera
.XXBinding
.name_display
Cube
,Camera
, andBinding
.RNA setter behaviour
name
name_display
name = prefix_for_ID_type + name_display
. So even when the old name wasQQSomethingWeird
, settingbinding.name_display = "NewName"
would effectively setbinding.name = "OBNewName"
(assuming it was already bound to some object earlier).Previously it was possible to "connect" a binding to an animated ID, which would (in pseudocode):
id.adt.binding_hande = binding.handle
binding.name = id.name
andbinding.idtype = GS(id.name)
.This was confusing, as it doesn't actually assign the
Animation
itself, and thus would cause some semi-assigned state. This function is no longer available.A nice aspect of this change is that all public C++ methods on the
animrig::Binding
class are nowconst
.Bindings now also always have a name. Previously it was possible to create bindings named
""
, but that's no longer possible.Bindings used to be renamed automatically when they were first assigned, for example from
XXBinding
toOBCube
. This behaviour has been removed, as it could potentially cause confusion.A future UI will use the binding's ID type to draw the appropriate icon. This will make it possible to distinguish bindings by type, so that
OBCamera
andCACamera
are still visually different.72f7f97e78
to4c841b6eba
WIP: Anim: Change how names of Bindings work, and how Bindings are created/assignedto Anim: Change how names of Bindings work, and how Bindings are created/assigned@blender-bot build
@blender-bot build
@ -397,0 +429,4 @@
/* If the binding is not yet tied to a type, use the ID name to give its initial name. This
* makes it possible to have bindings named "Binding.047" when they are created, and named
* after whatever is using them when first assigned. */
if (!binding->has_idtype()) {
Change this to a comparison (without suffix) of the default name, and only rename the Binding then. That'll allow setting up an Animation data-block with its bindings, and then assigning it everywhere.
@ -574,1 +603,4 @@
std::string Binding::name_prefix_for_idtype() const
{
if (this->idtype == 0) {
use
has_idtype()
@ -127,4 +128,3 @@
static AnimationBinding *rna_Animation_bindings_new(Animation *anim_id,
bContext *C,
ReportList *reports,
ID *animated_id)
rename
animated_id
tosuitable_for
, as that's the Python/RNA parameter name.Reading through this in review, I actually found
suitable_for_id
to be a confusing name. It reads like a bool. Maybe rename bothid_for_binding
?How about
for_id
? Because the callanim.bindings.new(for_id=C.object)
looks kinda nice to me.anim.bindings.new(id_for_binding=C.object)
seems a bit repetetive.For those reading along: we discussed face-to-face that
id_for_binding
is clearest in the C++ code, and the parameter for Python will befor_id
.@ -224,0 +244,4 @@
if (binding.has_idtype()) {
/* Check if the new name is going to be compatible with the already-established ID type. */
const StringRef name_ref(name);
const std::string old_prefix = binding.name_prefix_for_idtype();
old_prefix
is not the right name,prefix_for_idtype
would be better.Looks good to me. Just some nits that can be addressed while landing (or ignored if you don't agree).
@ -122,0 +127,4 @@
/**
* Create a new, unused Binding.
*
* When calling `assign_id(returned_binding, animated_id)`, the binding will be renamed to that
This behavior of
assign_id()
doesn't appear to be documented onassign_id()
itself.@ -169,0 +199,4 @@
void binding_name_ensure_prefix(Binding &binding);
/**
* Set the binding's name and ID type to those of the animated ID.
Document the behavior when the binding already has an ID type, since that's non-obvious without looking at the code right now.
It also appears (looking at the implementation) that the name is no longer assigned, aside from just ensuring it's not a duplicate.
@ -383,3 +420,3 @@
if (adt->animation) {
if (adt->animation && adt->animation != this) {
/* Unassign the ID from its existing animation first, or use the top-level
Nit: this phrasing reads as if it's a description of the code below, rather than a description of what should be done to avoid this case. Maybe just prepend "To avoid this case".
@ -575,0 +635,4 @@
StringRefNull Binding::name_without_prefix() const
{
/* Avoid accessing an uninitialised part of the string accidentally. */
if (this->name[0] == '\0' || this->name[1] == '\0') {
My initial reaction was that this shouldn't be possible, and should be turned into an assert. But since the user can directly set the whole name, that's what can cause this (if I'm understanding correctly).
Maybe make a short note of that, either here or where the name field is defined, since this departs from how name prefixes work in other parts of Blender (namely IDs).
The code should remain, as asserts are debug-only, and in release mode Blender should still be crash-free. But you have a good point, and I'll add some more asserts anyway + checks on the length in the RNA setters.
@ -258,0 +258,4 @@
WM_reportf(RPT_ERROR,
"Animation '%s' binding '%s' (%d) could not be assigned to %s",
anim->id.name + 2,
binding->name_without_prefix().c_str(),
It might make sense to print the full name here instead, or at least also print the type prefix separately. If the
Animation
has bothOBCube
andMECube
bindings, it may not be immediately obvious which one is being referred to, forcing the reader to do more thinking than needed to troubleshoot it.@blender-bot build