blender
/
blender
136
406
Fork 590
Code Issues 6.2k Pull Requests 764 Projects 19 Wiki Activity

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
Conversation 9 Commits 7 Files Changed 7 +22 -10
3 changed files with 22 additions and 10 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 76efd2b1b7 - Show all commits

3
source/blender/blenkernel/intern/fmodifier.c
View File

@ -1109,6 +1109,9 @@ FModifier *add_fmodifier(ListBase *modifiers, int type, FCurve *owner_fcu)
fcm->influence = 1.0f;
BLI_addtail(modifiers, fcm);
/* Copy Modifier name. */
Sybren A. Stüvel commented 2023-05-01 16:19:59 +02:00
Outdated
Review
Copy Link

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.
BLI_strncpy(fcm->name, fmi->name, sizeof(fmi->name));
Sybren A. Stüvel commented 2023-03-03 15:45:58 +01:00
Outdated
Review
Copy Link

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.
/* tag modifier as "active" if no other modifiers exist in the stack yet */
if (BLI_listbase_is_single(modifiers)) {
Sybren A. Stüvel commented 2023-04-17 14:35:26 +02:00
Outdated
Review
Copy Link

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?
fcm->flag |= FMODIFIER_FLAG_ACTIVE;

10
source/blender/editors/animation/fmodifier_ui.c
View File

@ -308,19 +308,15 @@ static void fmodifier_panel_header(const bContext *C, Panel *panel)
uiBlock *block = uiLayoutGetBlock(layout);
uiLayout *sub = uiLayoutRow(layout, true);
uiLayoutSetAlignment(sub, UI_LAYOUT_ALIGN_LEFT);
uiLayoutSetEmboss(sub, UI_EMBOSS_NONE);
/* Checkbox for 'active' status (for now). */
uiItemR(sub, ptr, "active", UI_ITEM_R_ICON_ONLY, "", ICON_NONE);
/* Name. */
if (fmi) {
uiItemL(sub, IFACE_(fmi->name), ICON_NONE);
}
else {
uiItemL(sub, IFACE_("<Unknown Modifier>"), ICON_NONE);
if (fmi && fcm->name[0] == '\0') {
BLI_strncpy(fcm->name, fmi->name, sizeof(fmi->name));
Sybren A. Stüvel commented 2023-03-03 16:48:02 +01:00
Outdated
Review
Copy Link

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.
Pratik Borhade commented 2023-04-14 14:52:17 +02:00
Outdated
Review
Copy Link

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?
}
uiItemR(sub, ptr, "name", 0, "", ICON_NONE);
/* Right align. */
sub = uiLayoutRow(layout, true);

19
source/blender/makesrna/intern/rna_fcurve.c
View File

@ -15,6 +15,8 @@
#include "BLT_translation.h"
#include "BLI_string_utils.h"
#include "BKE_action.h"
#include "RNA_access.h"
@ -758,6 +760,17 @@ static void rna_FModifier_update(Main *bmain, Scene *UNUSED(scene), PointerRNA *
rna_tag_animation_update(bmain, id);
}
static void rna_fModifier_name_set(PointerRNA *ptr, const char *value)
{
FModifier *fcm = (FModifier *)ptr->data;
BLI_strncpy(fcm->name, value, sizeof(fcm->name));
/* Check unique name. */
const FModifierTypeInfo *fmi = get_fmodifier_typeinfo(fcm->type);
Sybren A. Stüvel commented 2023-03-03 16:50:00 +01:00
Outdated
Review
Copy Link

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.
ListBase list = BLI_listbase_from_link((Link *)fcm);
BLI_uniquename(&list, fcm, fmi->name, '.', offsetof(FModifier, name), sizeof(fcm->name));
}
static void rna_FModifier_verify_data_update(Main *bmain, Scene *scene, PointerRNA *ptr)
{
FModifier *fcm = (FModifier *)ptr->data;
@ -1662,12 +1675,12 @@ static void rna_def_fmodifier(BlenderRNA *brna)
RNA_def_struct_refine_func(srna, "rna_FModifierType_refine");
RNA_def_struct_ui_text(srna, "F-Modifier", "Modifier for values of F-Curve");
# if 0 /* XXX not used yet */
/* name */
prop = RNA_def_property(srna, "name", PROP_STRING, PROP_NONE);
RNA_def_property_string_funcs(prop, NULL, NULL, "rna_fModifier_name_set");
RNA_def_property_ui_text(prop, "Name", "F-Curve Modifier name");
RNA_def_property_update(prop, NC_OBJECT | ND_MODIFIER | NA_RENAME, NULL);
RNA_def_struct_name_property(srna, prop);
RNA_def_property_ui_text(prop, "Name", "Short description of F-Curve Modifier");
# endif /* XXX not used yet */
/* type */
prop = RNA_def_property(srna, "type", PROP_ENUM, PROP_NONE);