Allow renaming F-curve modifier #104949

Merged
Pratik Borhade merged 7 commits from PratikPB2123/blender:T103855-rename-fmodifier into main 2023-05-02 13:07:55 +02:00

Modifier name is being displayed as label in panel header which has restricted
the editing of it. This PR will allow to change the name of F-curve modifier
by exposing string property "name"

Included versioning code for modifier name. This will add suffix to the name if
multiple instances of same modifier exists in modifier stack.

Differential Revision: https://archive.blender.org/developer/D17142

Modifier name is being displayed as label in panel header which has restricted the editing of it. This PR will allow to change the name of F-curve modifier by exposing string property "name" Included versioning code for modifier name. This will add suffix to the name if multiple instances of same modifier exists in modifier stack. Differential Revision: https://archive.blender.org/developer/D17142
Pratik Borhade added 1 commit 2023-02-19 13:25:59 +01:00
76efd2b1b7 Allow renaming F-curve modifier
Modifier name is being displayed as label in panel header which has restricted
the name editing of it. Patch will allow to change the name of F-curve modifier
by exposing string property "name"

Differential Revision: https://archive.blender.org/developer/D17142
Pratik Borhade requested review from Christoph Lendenfeld 2023-02-19 13:26:16 +01:00
Poster
Collaborator
  • When opening a file, that already had an fcurve modifier prior to the patch, the name field is empty
  • it's possible to have two modifiers with the same name

@ChrisLend hi, this PR includes previous two changes you requested


The latter is the bigger issue. We want to eventually get to the point of multi editing FCurve modifiers using the ALT key

Since this is not implemented yet, I'm not sure whether it has to be part of this patch.

> - When opening a file, that already had an fcurve modifier prior to the patch, the name field is empty > - it's possible to have two modifiers with the same name @ChrisLend hi, this PR includes previous two changes you requested ----- > The latter is the bigger issue. We want to eventually get to the point of multi editing FCurve modifiers using the ALT key Since this is not implemented yet, I'm not sure whether it has to be part of this patch.
Pratik Borhade added this to the Animation & Rigging project 2023-02-19 14:53:45 +01:00

hi @PratikPB2123
I think it would be good to make sure naming is unique regardless of what we plan to do in the future.
It would mean it's consistent with the rest of Blender

hi @PratikPB2123 I think it would be good to make sure naming is unique regardless of what we plan to do in the future. It would mean it's consistent with the rest of Blender
Poster
Collaborator

I think it would be good to make sure naming is unique

@ChrisLend hi, I've included the fix that makes modifier name unique. In case of same names, it adds .00x suffix (x=number)

> I think it would be good to make sure naming is unique @ChrisLend hi, I've included the fix that makes modifier name unique. In case of same names, it adds .00x suffix (x=number)
Christoph Lendenfeld requested changes 2023-02-23 10:28:44 +01:00
Christoph Lendenfeld left a comment
Collaborator

it has a unique name when renaming, but when creating 2 Noise modifiers both of them are still just called "Noise"

it has a unique name when renaming, but when creating 2 Noise modifiers both of them are still just called "Noise"

I think those are two different things (ensuring uniqueness, and allowing manual name changes), which should be in two different PRs. I'm all in favour of having unique names, by the way! That'll make the multi-FCurve modifier editing nicer as well.

I think those are two different things (ensuring uniqueness, and allowing manual name changes), which should be in two different PRs. I'm all in favour of having unique names, by the way! That'll make the multi-FCurve modifier editing nicer as well.

Question from @BClark during the module meeting: would this work for the NLA modifiers as well? Or is it limited to FCurve modifiers?

Question from @BClark during the module meeting: would this work for the NLA modifiers as well? Or is it limited to FCurve modifiers?
Poster
Collaborator

@dr.sybren yes, works for modifiers in NLA editor too.
see: nla-editor-modifier

@dr.sybren yes, works for modifiers in NLA editor too. see: [nla-editor-modifier](/attachments/9b72e702-bbee-4cd4-b111-5f3eba63e4ea)

I think those are two different things (ensuring uniqueness, and allowing manual name changes), which should be in two different PRs. I'm all in favour of having unique names, by the way! That'll make the multi-FCurve modifier editing nicer as well.

I have to come back on this. Forcing unique names without being able to edit them is kinda pointless, so I think this can still be done in the same PR.

> I think those are two different things (ensuring uniqueness, and allowing manual name changes), which should be in two different PRs. I'm all in favour of having unique names, by the way! That'll make the multi-FCurve modifier editing nicer as well. I have to come back on this. Forcing unique names without being able to edit them is kinda pointless, so I think this can still be done in the same PR.
Sybren A. Stüvel requested changes 2023-03-03 16:50:25 +01:00
Sybren A. Stüvel left a comment
Collaborator

The functionality is very welcome, but I do have some notes about the code.

The functionality is very welcome, but I do have some notes about the code.
@ -1110,2 +1110,4 @@
BLI_addtail(modifiers, fcm);
/* Copy Modifier name. */
BLI_strncpy(fcm->name, fmi->name, sizeof(fmi->name));

This should be using sizeof(fcm->name); the limit should be determined by the available memory to copy into. Same for similar calls below.

This should be using `sizeof(fcm->name)`; the limit should be determined by the available memory to copy **into**. Same for similar calls below.
@ -321,2 +316,2 @@
else {
uiItemL(sub, IFACE_("<Unknown Modifier>"), ICON_NONE);
if (fmi && fcm->name[0] == '\0') {
BLI_strncpy(fcm->name, fmi->name, sizeof(fmi->name));

Panel drawing code shouldn't alter the drawn information. If fcm->name must be set, do so in the appropriate place.

Panel drawing code shouldn't alter the drawn information. If `fcm->name` must be set, do so in the appropriate place.
Poster
Collaborator

When opening a file, that already had an fcurve modifier prior to the patch, the name field is empty

Hi, sorry for the late update. Included all fixes in branch expect this one.
I'm not sure what would be the correct place to set fcm->name.
Any clue?

> When opening a file, that already had an fcurve modifier prior to the patch, the name field is empty Hi, sorry for the late update. Included all fixes in branch expect this one. I'm not sure what would be the correct place to set `fcm->name`. Any clue?
@ -761,0 +766,4 @@
BLI_strncpy(fcm->name, value, sizeof(fcm->name));
/* Check unique name. */
const FModifierTypeInfo *fmi = get_fmodifier_typeinfo(fcm->type);

This shouldn't be done in an RNA function, but rather in a function like BKE_fmodifier_name_set(). That can then be called from here and from other places where the functionality is needed.

This shouldn't be done in an RNA function, but rather in a function like `BKE_fmodifier_name_set()`. That can then be called from here and from other places where the functionality is needed.
Pratik Borhade added 2 commits 2023-04-14 14:49:50 +02:00
8f66da6088 Include following fixes:
- Ensure uniqueness of names when creating modifiers
- Use `fcm->name` instead of `fmi->name` for max size
- New function `BKE_fmodifier_unique_name_set` to check unique modifier names
- Revert change from `fmodifier_ui.c` which was setting `fcm->name`
Sybren A. Stüvel requested changes 2023-04-17 14:39:19 +02:00
Sybren A. Stüvel left a comment
Collaborator

The PR is getting in better shape. I think with a simple change it can be improved a bit.

Besides this, I think we also need to discuss how we're going to handle versioning of the modifier names. I'll discuss that in the module channel, or during a module meeting, and report back here.

The PR is getting in better shape. I think with a simple change it can be improved a bit. Besides this, I think we also need to discuss how we're going to handle versioning of the modifier names. I'll discuss that in the module channel, or during a module meeting, and report back here.
@ -1112,0 +1113,4 @@
BLI_strncpy(fcm->name, fmi->name, sizeof(fcm->name));
/* Check unique name. */
BKE_fmodifier_unique_name_set(fcm);

I think a function call BKE_fmodifier_name_set(fcm, fmi->name) would be preferrable. That could combine the BLI_strncpy() and the BLI_uniquename() calls, and give the caller a one-stop-shop for setting the modifier name. That way the caller doesn't need to know anything about the FModifier struct; limiting required knowledge is often a good idea.

That function would also need to have some documentation about how it would behave when there already is a modifier with the given name. Would it change the name of fcm to become unique? Or of the other modifier with that same name?

I think a function call `BKE_fmodifier_name_set(fcm, fmi->name)` would be preferrable. That could combine the `BLI_strncpy()` and the `BLI_uniquename()` calls, and give the caller a one-stop-shop for setting the modifier name. That way the caller doesn't need to know anything about the `FModifier` struct; limiting required knowledge is often a good idea. That function would also need to have some documentation about how it would behave when there already is a modifier with the given name. Would it change the name of `fcm` to become unique? Or of the other modifier with that same name?
Pratik Borhade added 1 commit 2023-04-18 11:27:25 +02:00
dbb1b162ef Use single function to set name and check uniqueness
- `BKE_fmodifier_name_set` function created
Pratik Borhade added 1 commit 2023-04-18 13:30:29 +02:00
91e27b96eb Add versioning code to name fcurve modifiers when opening old files.
This change is needed. Otherwise modifier would have empty name field
and duplicate names.
Pratik Borhade requested review from Christoph Lendenfeld 2023-04-18 15:38:46 +02:00
Pratik Borhade requested review from Sybren A. Stüvel 2023-04-18 15:38:52 +02:00

When playing around with current Blender (without this PR), I encountered #107086 when trying to index into the fcurve modifier stack by modifier name. The cause was that the default string indexing code looks for nameproperty (not name), and fcurve modifiers don't have nameproperty set to anything. Which resulted in a null pointer dereference.

The fix in #107086 ensures that Blender won't crash in these cases anymore... but maybe we shouldn't be using name for the editable modifier names, but rather nameproperty? Either that, or it looks like a custom callback function can be created that pyrna_prop_collection_subscript_str() will then use for string lookups instead.

Not sure what the right fix is, but figured it was worth mentioning here, since it seems related.

When playing around with current Blender (without this PR), I encountered #107086 when trying to index into the fcurve modifier stack by modifier name. The cause was that the default string indexing code looks for `nameproperty` (not `name`), and fcurve modifiers don't have `nameproperty` set to anything. Which resulted in a null pointer dereference. The fix in #107086 ensures that Blender won't crash in these cases anymore... but maybe we shouldn't be using `name` for the editable modifier names, but rather `nameproperty`? Either that, or it looks like a [custom callback function](https://projects.blender.org/blender/blender/src/commit/0e23aef6b6dc000aecdd3e9a780a1e0602e83e2e/source/blender/python/intern/bpy_rna.c#L2358-L2362) can be created that `pyrna_prop_collection_subscript_str()` will then use for string lookups instead. Not sure what the right fix is, but figured it was worth mentioning here, since it seems related.
Christoph Lendenfeld reviewed 2023-04-28 16:37:43 +02:00
Christoph Lendenfeld left a comment
Collaborator

Tested again and no more complaints from me :)
Do check what @cessen said though. I think changing the name shouldn't be too hard

Tested again and no more complaints from me :) Do check what @cessen said though. I think changing the name shouldn't be too hard
Christoph Lendenfeld approved these changes 2023-04-28 16:38:16 +02:00
Christoph Lendenfeld left a comment
Collaborator

once again, I forgot that ctrl+enter just sends a comment...

once again, I forgot that ctrl+enter just sends a comment...
Sybren A. Stüvel requested changes 2023-05-01 16:23:42 +02:00
Sybren A. Stüvel left a comment
Collaborator

It works well! Just some small remarks about doc quality.

It works well! Just some small remarks about doc quality.
@ -1109,6 +1109,9 @@ FModifier *add_fmodifier(ListBase *modifiers, int type, FCurve *owner_fcu)
fcm->influence = 1.0f;
BLI_addtail(modifiers, fcm);
/* Set modifier name. */

This comment just basically repeats the function name. It would be better to explain why this is done here: to ensure the name is unique within the list of modifiers, it can only be set after it has been added to the list.

This comment just basically repeats the function name. It would be better to explain *why* this is done here: to ensure the name is unique within the list of modifiers, it can only be set *after* it has been added to the list.
@ -4284,2 +4284,4 @@
}
/* Set fcurve modifier name and check their uniqueness when opening old files. Otherwise
* modifiers would have empty name field. */

It's unclear how calling BKE_fmodifier_name_set(fcm, "") (so with an empty name parameter) would avoid modifiers having an empty name.

If this triggers some kind of special behaviour of BKE_fmodifier_name_set(), that should be mentioned in that function's documentation.

It's unclear how calling `BKE_fmodifier_name_set(fcm, "")` (so with an empty `name` parameter) would avoid modifiers having an empty name. If this triggers some kind of special behaviour of `BKE_fmodifier_name_set()`, that should be mentioned in that function's documentation.
Poster
Collaborator

It's unclear how calling BKE_fmodifier_name_set(fcm, "") (so with an empty name parameter) would avoid modifiers having an empty name.

Updated code comments. This is handled within BLI_uniquename (see defname parameter).

> It's unclear how calling BKE_fmodifier_name_set(fcm, "") (so with an empty name parameter) would avoid modifiers having an empty name. Updated code comments. This is handled within BLI_uniquename (see defname parameter).
Sybren A. Stüvel reviewed 2023-05-01 16:31:29 +02:00
@ -240,6 +240,9 @@ void BKE_fcurves_free(ListBase *list);
*/
void BKE_fcurves_copy(ListBase *dst, ListBase *src);
/* Set fcurve modifier name and check uniqueness. */

"check" → "ensure"

To "check" doesn't necessarily imply that this uniqueness is ensured. It could also just return a boolean. And the return type of the function shouldn't be necessary to understand its documentation.

"check" → "ensure" To "check" doesn't necessarily imply that this uniqueness is ensured. It could also just return a boolean. And the return type of the function shouldn't be necessary to understand its documentation.
Pratik Borhade added 1 commit 2023-05-02 11:03:41 +02:00
Sybren A. Stüvel approved these changes 2023-05-02 11:18:06 +02:00
Sybren A. Stüvel left a comment
Collaborator

Just one tiny grammar note, otherwise LGTM!

No need to re-review after addressing this. Thanks again, this PR is super useful :)

Just one tiny grammar note, otherwise LGTM! No need to re-review after addressing this. Thanks again, this PR is super useful :)
@ -241,2 +241,4 @@
void BKE_fcurves_copy(ListBase *dst, ListBase *src);
/* Set fcurve modifier name and ensure uniqueness.
* Pass new name string when its edited otherwise pass empty string. */

"its" → "it's been"

"its" → "it's been"
Pratik Borhade added 1 commit 2023-05-02 11:25:59 +02:00
Pratik Borhade merged commit 0001485365 into main 2023-05-02 13:07:55 +02:00
Pratik Borhade deleted branch T103855-rename-fmodifier 2023-05-02 13:07:56 +02:00
Poster
Collaborator

Merged, hanks for reviewing :)

Merged, hanks for reviewing :)
Howard Trickey referenced this issue from a commit 2023-05-29 02:51:43 +02:00
Demeter Dzadik removed this from the Animation & Rigging project 2023-08-28 19:12:07 +02:00

Reviewers

Christoph Lendenfeld approved these changes 2023-04-28 16:38:16 +02:00
Sybren A. Stüvel approved these changes 2023-05-02 11:18:06 +02:00
Sign in to join this conversation.
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 & 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
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
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
4 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#104949
There is no content yet.