WIP: Action+Slot selector in Properties panels #122500

Draft
Sybren A. Stüvel wants to merge 13 commits from dr.sybren/blender:anim/baklava-binding-selector into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
  • Rename the enum property AnimData.action_slot to .action_slot_enum to move it out of the way.
  • Add a new pointer property AnimData.action_slot
  • Use template_search() to show the selector
  • Add an Action + Slot selector to the Camera/Mesh/Lattice/someother Properties panels.

For an overview of the resulting API, see #124399

There are a few issues illuminated by the Camera Properties panel:

  • If camera.animation_data is None, there is no way to call layout.template_ID(camera.animation_data, 'action') to show the selector. The Action editor works around this, by having an action property in the space data type. This feels a bit hackish though, and I want to find an alternative solution that is easier to use in other places / spaces. In the end, I copied the Action editor approach (except it's runtime data rather than saved to DNA, so slightly less hackish).
  • The template_search() UI is still showing the 'name property' of the ActionSlot RNA struct. This property is meant as key in the action.slots collection, though. UI should show slot.name_display, but distinguishing 'collection key name' from 'display name' is not yet possible in Blender. Update: showing the display name has been hacked in.
  • Creating a new slot and unassigning a slot both have to be implemented as operators. Since operators (AFAIK) cannot get PointerProperties, this means that we may need two concrete operators for this for each data type (camera, mesh, etc). I'm not fan, so I want to see if this can be worked around somehow. A good workaround could be to have these operators for just the properties editor.

Example of the Animation panel:

- Rename the enum property `AnimData.action_slot` to `.action_slot_enum` to move it out of the way. - Add a new pointer property `AnimData.action_slot` - Use `template_search()` to show the selector - Add an Action + Slot selector to the Camera/Mesh/Lattice/someother Properties panels. For an overview of the resulting API, see #124399 There are a few issues illuminated by the Camera Properties panel: - [x] If `camera.animation_data` is `None`, there is no way to call `layout.template_ID(camera.animation_data, 'action')` to show the selector. The Action editor works around this, by having an `action` property in the space data type. This feels a bit hackish though, and I want to find an alternative solution that is easier to use in other places / spaces. In the end, I copied the Action editor approach (except it's runtime data rather than saved to DNA, so slightly less hackish). - [ ] The `template_search()` UI is still showing the 'name property' of the `ActionSlot` RNA struct. This property is meant as key in the `action.slots` collection, though. UI should show `slot.name_display`, but distinguishing 'collection key name' from 'display name' is not yet possible in Blender. **Update:** showing the display name has been hacked in. - [ ] Creating a new slot and unassigning a slot both have to be implemented as operators. ~~Since operators (AFAIK) cannot get PointerProperties, this means that we may need two concrete operators for this for each data type (camera, mesh, etc). I'm not fan, so I want to see if this can be worked around somehow.~~ A good workaround could be to have these operators for just the properties editor. Example of the Animation panel: ![](https://projects.blender.org/attachments/02bdc75e-6b62-4294-aaf1-234df6b453b2)
Sybren A. Stüvel added 1 commit 2024-05-30 16:26:55 +02:00
- Rename the enum property `AnimData.action_binding` to
  `.action_binding_enum` to move it out of the way.
- Add a new pointer property `AnimData.action_binding`
- Use `template_search()` to show the selector

This has a few issues though:

- The `Binding.name` property is used for display & edit of the name.
  This should be using `name_display` instead.
- It doesn't seem to be possible to show an icon.
- `template_search()` eats up reports, so that choosing an incompatible
  binding is silently ignored (instead of showing the actual message).
Sybren A. Stüvel changed title from WIP on a 'binding' selector in the UI to WIP: 'binding' selector in the UI 2024-05-30 16:28:51 +02:00
Sybren A. Stüvel added the
Module
Animation & Rigging
label 2024-05-30 16:29:51 +02:00
Member
  • It doesn't seem to be possible to show an icon.

It should work if the RNA struct has an icon associated via RNA_def_struct_ui_icon().

  • The Binding.name property is used for display & edit of the name. This should be using name_display instead. name encodes the icon + name_display, in the same vein as how ID.name encodes its type + its name.

This seems like a inconsistent design choice. For IDs we don't expose the name with the prefix. Name and ID-type are two separate RNA properties. This seems quite reasonable to follow?

If this is not an option, the template uses whatever is defined as the struct's name property using RNA_def_struct_name_property(). Seems like this should be used for name_display instead?

  • template_search() eats up reports, so that choosing an incompatible binding is silently ignored (instead of showing the actual message).

Not sure which reports it eats up, operator reports?

> - It doesn't seem to be possible to show an icon. It should work if the RNA struct has an icon associated via `RNA_def_struct_ui_icon()`. > - The `Binding.name` property is used for display & edit of the name. This should be using `name_display` instead. `name` encodes the icon + `name_display`, in the same vein as how `ID.name` encodes its type + its name. This seems like a inconsistent design choice. For IDs we don't expose the name with the prefix. Name and ID-type are two separate RNA properties. This seems quite reasonable to follow? If this is not an option, the template uses whatever is defined as the struct's name property using `RNA_def_struct_name_property()`. Seems like this should be used for `name_display` instead? > - `template_search()` eats up reports, so that choosing an incompatible binding is silently ignored (instead of showing the actual message). Not sure which reports it eats up, operator reports?
Author
Member
  • It doesn't seem to be possible to show an icon.

It should work if the RNA struct has an icon associated via RNA_def_struct_ui_icon().

That doesn't seem to have a callback. I want the icon of the binding to reflect the kind of data it is meant for.

  • The Binding.name property is used for display & edit of the name. This should be using name_display instead. name encodes the icon + name_display, in the same vein as how ID.name encodes its type + its name.

This seems like a inconsistent design choice. For IDs we don't expose the name with the prefix. Name and ID-type are two separate RNA properties. This seems quite reasonable to follow?

The issue is that the IDs are stored in different collections (in Main), depending on their type. This means that ID.name[2:] is always unique within that collection. Bindings of a single Action are all stored in a flat list on that Action, and not split out per ID type. This is why I put the ID code in the name, to ensure that we can have an object "Camera" and a camera "Camera", both animated with a binding with display name "Camera". Those bindings will be named "OBCamera" and "CACamera" internally.

The icon shows the differences. This works with the enum property:

image

If this is not an option, the template uses whatever is defined as the struct's name property using RNA_def_struct_name_property(). Seems like this should be used for name_display instead?

I'll give that a try. It would break the action.bindings[binding.thenameproperty] == binding relation. No idea how important that is, though.

  • template_search() eats up reports, so that choosing an incompatible binding is silently ignored (instead of showing the actual message).

Not sure which reports it eats up, operator reports?

Setter reports, in this case the ones from rna_AnimData_action_binding_set():

  if (!action.assign_id(&binding, *animated_id)) {
    /* TODO: make assign_id() return a different type that gives us more info about what went
     * wrong. */
    BKE_reportf(
        reports, RPT_ERROR, "Cannot assign binding %s to %s,", binding.name, animated_id->name);
    return;
  }

The report is printed to stdout but not shown in the UI.

> > - It doesn't seem to be possible to show an icon. > > It should work if the RNA struct has an icon associated via `RNA_def_struct_ui_icon()`. That doesn't seem to have a callback. I want the icon of the binding to reflect the kind of data it is meant for. > > - The `Binding.name` property is used for display & edit of the name. This should be using `name_display` instead. `name` encodes the icon + `name_display`, in the same vein as how `ID.name` encodes its type + its name. > > This seems like a inconsistent design choice. For IDs we don't expose the name with the prefix. Name and ID-type are two separate RNA properties. This seems quite reasonable to follow? The issue is that the IDs are stored in different collections (in `Main`), depending on their type. This means that `ID.name[2:]` is always unique within that collection. Bindings of a single Action are all stored in a flat list on that Action, and not split out per ID type. This is why I put the ID code in the name, to ensure that we can have an object "Camera" and a camera "Camera", both animated with a binding with display name "Camera". Those bindings will be named "OBCamera" and "CACamera" internally. The icon shows the differences. This works with the enum property: ![image](/attachments/483e791d-84a4-4c96-83a3-fddff9ae498c) > If this is not an option, the template uses whatever is defined as the struct's name property using `RNA_def_struct_name_property()`. Seems like this should be used for `name_display` instead? I'll give that a try. It would break the `action.bindings[binding.thenameproperty] == binding` relation. No idea how important that is, though. > > - `template_search()` eats up reports, so that choosing an incompatible binding is silently ignored (instead of showing the actual message). > > Not sure which reports it eats up, operator reports? Setter reports, in this case the ones from `rna_AnimData_action_binding_set()`: ```cpp if (!action.assign_id(&binding, *animated_id)) { /* TODO: make assign_id() return a different type that gives us more info about what went * wrong. */ BKE_reportf( reports, RPT_ERROR, "Cannot assign binding %s to %s,", binding.name, animated_id->name); return; } ``` The report is printed to stdout but not shown in the UI.
Sybren A. Stüvel added 1 commit 2024-05-30 17:38:04 +02:00
Sybren A. Stüvel added 2 commits 2024-05-31 16:00:20 +02:00
Author
Member

@JulianEisel I've added a new collection property AnimData.action_bindings that only contains the bindings that are suitable for the animated ID. That makes the icon less pressing, as it's no longer necessary to distinguish between (in the above example) the two 'Camera' bindings.

@JulianEisel I've added a new collection property `AnimData.action_bindings` that only contains the bindings that are suitable for the animated ID. That makes the icon less pressing, as it's no longer necessary to distinguish between (in the above example) the two 'Camera' bindings.
Sybren A. Stüvel added 3 commits 2024-06-03 15:34:04 +02:00
Add a temp icon for action bindings. This is now just the "link" icon,
which shouldn't be reused for this.
This is a proof-of-concept panel, to figure out how to present the
Action selector in object data property panels.
Sybren A. Stüvel added 2 commits 2024-06-04 18:15:08 +02:00
Sybren A. Stüvel added 1 commit 2024-06-06 16:33:37 +02:00
Sybren A. Stüvel changed title from WIP: 'binding' selector in the UI to WIP: Action + Binding selector in Camera Properties panel 2024-06-10 11:47:07 +02:00
Sybren A. Stüvel added 5 commits 2024-06-10 16:14:27 +02:00
Sybren A. Stüvel added 1 commit 2024-06-10 16:24:44 +02:00
Sybren A. Stüvel force-pushed anim/baklava-binding-selector from 98dc07fb60 to 601dfc5937 2024-07-08 17:07:07 +02:00 Compare
Sybren A. Stüvel added 1 commit 2024-07-08 17:57:17 +02:00
Sybren A. Stüvel added 1 commit 2024-07-08 18:13:27 +02:00
This is in no way intended to be final code, just committing it to share
with people to find a better approach.
Sybren A. Stüvel changed title from WIP: Action + Binding selector in Camera Properties panel to WIP: Action+Slot selector in Camera Properties panel 2024-07-08 18:13:37 +02:00
Sybren A. Stüvel added 2 commits 2024-07-09 15:00:35 +02:00
Sybren A. Stüvel added 1 commit 2024-07-12 12:39:02 +02:00
Sybren A. Stüvel added 2 commits 2024-07-12 15:56:39 +02:00
UI: show Animation panel in many more places
Some checks reported errors
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
921b432672
Sybren A. Stüvel changed title from WIP: Action+Slot selector in Camera Properties panel to WIP: Action+Slot selector in Properties panels 2024-07-12 15:59:48 +02:00
Sybren A. Stüvel added 1 commit 2024-07-12 16:54:30 +02:00
Merge remote-tracking branch 'origin/main' into anim/baklava-binding-selector
All checks were successful
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
ccd7072c00
Author
Member

@blender-bot build

@blender-bot build
Author
Member

This PR is now waiting for !124598 to land. After that I'll do another merge with main, and then we can create a test build for animators to play around with.

This PR is now waiting for !124598 to land. After that I'll do another merge with `main`, and then we can create a test build for animators to play around with.
Sybren A. Stüvel added 1 commit 2024-07-15 10:44:13 +02:00
Merge remote-tracking branch 'origin/main' into anim/baklava-binding-selector
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
43a1897519
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR122500) when ready.
Sybren A. Stüvel added 1 commit 2024-07-15 11:45:23 +02:00
Sybren A. Stüvel added 1 commit 2024-07-16 16:23:41 +02:00
Merge remote-tracking branch 'origin/main' into anim/baklava-binding-selector
Some checks reported errors
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
3c586b20c6
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR122500) when ready.
Sybren A. Stüvel added 1 commit 2024-07-16 16:51:55 +02:00
Fix: #include "RNA_prototypes.hh" (it moved to C++)
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
c980db6299
Author
Member

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR122500) when ready.
Member

Was asked to give feedback on the special handling done here, like:

if (itemptr.type == &RNA_ActionSlot) {
    PropertyRNA *prop = RNA_struct_find_property(&itemptr, "name_display");
   /* ... */
}

While not nice, we already do this in a few places. These hacks are trivial and local, not really worth adding options to the general API for such one-off corner cases. Should we need it in more cases, we can still do it through the API (e.g. here by passing an optional name property identifier).
Maybe worth noting the alternative solution in a comment.

Was asked to give feedback on the special handling done here, like: ```Cpp if (itemptr.type == &RNA_ActionSlot) { PropertyRNA *prop = RNA_struct_find_property(&itemptr, "name_display"); /* ... */ } ``` While not nice, we already do this in a few places. These hacks are trivial and local, not really worth adding options to the general API for such one-off corner cases. Should we need it in more cases, we can still do it through the API (e.g. here by passing an optional name property identifier). Maybe worth noting the alternative solution in a comment.
Some checks failed
buildbot/vexp-code-patch-lint Build done.
buildbot/vexp-code-patch-darwin-x86_64 Build done.
buildbot/vexp-code-patch-darwin-arm64 Build done.
buildbot/vexp-code-patch-linux-x86_64 Build done.
buildbot/vexp-code-patch-windows-amd64 Build done.
buildbot/vexp-code-patch-coordinator Build done.
This pull request is marked as a work in progress.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u anim/baklava-binding-selector:dr.sybren-anim/baklava-binding-selector
git checkout dr.sybren-anim/baklava-binding-selector
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
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
Asset Browser Project
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
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#122500
No description provided.