Fix #125426: Update paths in animation when renaming IDProperty #125474

Merged
YimingWu merged 8 commits from ChengduLittleA/blender:fix-125426 into main 2024-08-15 13:56:39 +02:00
Member

Previously when renaming an IDProperty the existing paths in the
animation/driver system isn't updated, this leads to missing animation
after renaming the property. Now BKE_animdata_fix_paths_rename_all
will be called so the animation system records the updated name.

Previously when renaming an IDProperty the existing paths in the animation/driver system isn't updated, this leads to missing animation after renaming the property. Now `BKE_animdata_fix_paths_rename_all` will be called so the animation system records the updated name.
YimingWu added the
Module
Animation & Rigging
label 2024-07-26 10:10:29 +02:00
YimingWu added 1 commit 2024-07-26 10:10:40 +02:00
Previously when renaming an IDProperty the existing paths in the
animation/driver system isn't updated, this leads to missing animation
after renaming the property. Now `BKE_animdata_fix_paths_rename_all`
will be called so the animation system records the updated name.
YimingWu requested review from Sybren A. Stüvel 2024-07-26 10:45:15 +02:00
YimingWu added 1 commit 2024-07-26 10:46:17 +02:00
Sybren A. Stüvel requested changes 2024-07-30 09:45:22 +02:00
Sybren A. Stüvel left a comment
Member

It works, thanks!

Just a few more or less cosmetic notes.

It works, thanks! Just a few more or less cosmetic notes.
@ -1882,3 +1882,3 @@
del item[name_old]
item[name] = new_value

Configure your IDE to do auto-formatting on save ;-)

Configure your IDE to do auto-formatting on save ;-)
ChengduLittleA marked this conversation as resolved
@ -1888,1 +1888,4 @@
if name_old != name:
item.id_data.fix_paths_rename_all(prefix="",old_name=name_old,
new_name=bpy.utils.escape_identifier(name))

It's not necessary to call bpy.utils.escape_identifier() here. Eventually BKE_animdata_fix_paths_rename() is called, which already handles escaping of the name when necessary.

It's not necessary to call `bpy.utils.escape_identifier()` here. Eventually `BKE_animdata_fix_paths_rename()` is called, which already handles escaping of the name when necessary.
ChengduLittleA marked this conversation as resolved
@ -656,0 +659,4 @@
const char *oldName,
const char *newName)
{
BKE_animdata_fix_paths_rename_all(id, prefix, oldName, newName);

In this case it's better to avoid BKE_animdata_fix_paths_rename_all as it has this little snippet:

  Main *bmain = G.main; /* XXX UGLY! */

Instead, call BKE_animdata_fix_paths_rename_all_ex() and pass it a bmain. You can get that bmain from RNA by adding RNA_def_function_flag(func, FUNC_USE_MAIN); below where you define the RNA function.

In this case it's better to avoid `BKE_animdata_fix_paths_rename_all` as it has this little snippet: ```cpp Main *bmain = G.main; /* XXX UGLY! */ ``` Instead, call `BKE_animdata_fix_paths_rename_all_ex()` and pass it a `bmain`. You can get that `bmain` from RNA by adding `RNA_def_function_flag(func, FUNC_USE_MAIN);` below where you define the RNA function.
ChengduLittleA marked this conversation as resolved
@ -1553,6 +1562,7 @@ static void rna_def_ID_properties(BlenderRNA *brna)
{
StructRNA *srna;
PropertyRNA *prop;
FunctionRNA *func;

This variable is unused.

This variable is unused.
ChengduLittleA marked this conversation as resolved
@ -2512,6 +2522,12 @@ static void rna_def_ID(BlenderRNA *brna)
func, "preview_image", "ImagePreview", "", "The existing or created preview");
RNA_def_function_return(func, parm);
func = RNA_def_function(srna, "fix_paths_rename_all", "rna_id_animdata_fix_paths_rename_all");

I think it's better to define this function on the AnimData struct directly.

I think it's better to define this function on the `AnimData` struct directly.
Author
Member

Hummm we kinda need that ref_id when calling rename. (Will FUNC_USE_SELF_ID do the job / will that be the ID we wanted?)

Hummm we kinda need that `ref_id` when calling rename. (Will `FUNC_USE_SELF_ID` do the job / will that be the ID we wanted?)
Member

Re: FUNC_USE_SELF_ID

I'm not 100% sure (RNA is still something I'm learning). But poking around for other examples I found rna_Driver_new() and rna_Driver_remove() in rna_animation.cc. Both are defined on animdata, but one of them uses FUNC_USE_SELF_ID and the other doesn't. From looking at those, it looks to me like FUNC_USE_SELF_ID does indeed provide access to the appropriate ID. And the doc comment on FUNC_USE_SELF_ID itself also suggests that that would be the case.

So I'd say give it a shot and see if it works.

Re: `FUNC_USE_SELF_ID` I'm not 100% sure (RNA is still something I'm learning). But poking around for other examples I found `rna_Driver_new()` and `rna_Driver_remove()` in `rna_animation.cc`. Both are defined on animdata, but one of them uses `FUNC_USE_SELF_ID` and the other doesn't. From looking at those, it looks to me like `FUNC_USE_SELF_ID` does indeed provide access to the appropriate ID. And the doc comment on `FUNC_USE_SELF_ID` itself also suggests that that would be the case. So I'd say give it a shot and see if it works.
ChengduLittleA marked this conversation as resolved
@ -2514,1 +2524,4 @@
func = RNA_def_function(srna, "fix_paths_rename_all", "rna_id_animdata_fix_paths_rename_all");
RNA_def_string(func, "prefix", 0, 128, "Prefix", "Name prefix");
RNA_def_string(func, "old_name", 0, 128, "Old Name", "Old name");
  • Don't use 0 for a const char *default_value parameter, use nullptr instead.
  • Where does the 128 come from?
- Don't use `0` for a `const char *default_value` parameter, use `nullptr` instead. - Where does the `128` come from?
Author
Member

I have no idea how long these should be 😅 (I guess I'll put MAX_ID_NAME here then)

I have no idea how long these should be 😅 (I guess I'll put `MAX_ID_NAME` here then)
Member

It looks to me like MAX_IDPROP_NAME is probably the right one to use here.

It looks to me like `MAX_IDPROP_NAME` is probably the right one to use here.
Member

Not sure if I accidentally marked this as resolved or if you did, but it looks like you haven't changed MAX_ID_NAME to MAX_IDPROP_NAME yet, so unresolving.

(No big deal or anything, just want to make sure this doesn't get lost.)

Not sure if I accidentally marked this as resolved or if you did, but it looks like you haven't changed `MAX_ID_NAME` to `MAX_IDPROP_NAME` yet, so unresolving. (No big deal or anything, just want to make sure this doesn't get lost.)
Member

Oooh, wait, it looks like the issue is that the version of this function on ID is still left over (after you added it to AnimData). And that's what I was looking at. I guess you forgot to delete it?

The version on AnimData is correct.

Oooh, wait, it looks like the issue is that the version of this function on ID is still left over (after you added it to AnimData). And that's what I was looking at. I guess you forgot to delete it? The version on AnimData is correct.
Author
Member

Ah I guess it's due to comment reference line being outdated 😅 it's kinda confusing sometimes indeed. But yeah it's changed to MAX_IDPROP_NAME already :D

Ah I guess it's due to comment reference line being outdated 😅 it's kinda confusing sometimes indeed. But yeah it's changed to `MAX_IDPROP_NAME` already :D
Member

👍

It looks like you forgot to delete this (now duplicate) version of rna_id_animdata_fix_paths_rename_all() that these comments are still referencing, though?

👍 It looks like you forgot to delete this (now duplicate) version of `rna_id_animdata_fix_paths_rename_all()` that these comments are still referencing, though?
Member

In fact, I think(?) all the changes to rna_ID.cc can now be discarded, since they've been moved into rna_animation_api.cc now.

In fact, I think(?) all the changes to rna_ID.cc can now be discarded, since they've been moved into rna_animation_api.cc now.
Author
Member

Weird... Actually I have no idea why it's still there. I'll double check

Weird... Actually I have no idea why it's still there. I'll double check
nathanvegdahl marked this conversation as resolved
@ -2515,0 +2526,4 @@
RNA_def_string(func, "prefix", 0, 128, "Prefix", "Name prefix");
RNA_def_string(func, "old_name", 0, 128, "Old Name", "Old name");
RNA_def_string(func, "new_name", 0, 128, "New Name", "New name");
RNA_def_function_ui_description(func, "Rename animation properties");

This description could use some work, "animation properties" is too vague. This will become the Python API documentation, and thus should be understandable without knowing which internal Blender function it calls.

This description could use some work, "animation properties" is too vague. This will become the Python API documentation, and thus should be understandable without knowing which internal Blender function it calls.
ChengduLittleA marked this conversation as resolved
Sybren A. Stüvel requested review from Nathan Vegdahl 2024-07-30 09:47:44 +02:00
YimingWu added 2 commits 2024-07-31 04:15:14 +02:00
YimingWu added 1 commit 2024-07-31 05:04:40 +02:00
YimingWu added 1 commit 2024-08-05 14:16:43 +02:00
YimingWu added 1 commit 2024-08-05 14:19:43 +02:00
YimingWu added 1 commit 2024-08-05 17:33:06 +02:00
Member

Testing the latest version of this PR, it appears that it no longer fixes the issue. Maybe FUNC_USE_SELF_ID doesn't provide the right ID after all? Or maybe it's something else.

Testing the latest version of this PR, it appears that it no longer fixes the issue. Maybe `FUNC_USE_SELF_ID` doesn't provide the right ID after all? Or maybe it's something else.
Author
Member

@nathanvegdahl wait it doesn't? seems to be working here, let me try again 😅 (tested again, seems to be still working?)

@nathanvegdahl wait it doesn't? seems to be working here, let me try again 😅 (tested again, seems to be still working?)
Member

Gah! My bad. I had the the new animation system experimental flag enabled, so it was using new-style actions. With that flag disabled, everything works. Sorry about that!

Gah! My bad. I had the the new animation system experimental flag enabled, so it was using new-style actions. With that flag disabled, everything works. Sorry about that!
Nathan Vegdahl approved these changes 2024-08-06 11:07:47 +02:00
Author
Member

@dr.sybren can we merge this? :D

@dr.sybren can we merge this? :D
Member

@dr.sybren is away from the office for a while. The code looks good to me and works in testing, and you've addressed his comments from his initial review. So let's merge.

@dr.sybren is away from the office for a while. The code looks good to me and works in testing, and you've addressed his comments from his initial review. So let's merge.
Author
Member

@nathanvegdahl ah great. Thanks!

@nathanvegdahl ah great. Thanks!
YimingWu merged commit 4f4add5406 into main 2024-08-15 13:56:39 +02:00
YimingWu deleted branch fix-125426 2024-08-15 13:56:43 +02:00
Sign in to join this conversation.
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
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
Viewport & EEVEE
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
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
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
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#125474
No description provided.