blender
/
blender
136
406
Fork 589
Code Issues 6.3k Pull Requests 766 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 +11 -7
4 changed files with 11 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 660fcd9b60 - Show all commits

3
source/blender/blenkernel/BKE_fcurve.h
View File

@ -240,7 +240,8 @@ void BKE_fcurves_free(ListBase *list);
*/
void BKE_fcurves_copy(ListBase *dst, ListBase *src);
/* Set fcurve modifier name and check uniqueness. */
/* Set fcurve modifier name and ensure uniqueness.
Sybren A. Stüvel commented 2023-05-01 16:31:29 +02:00
Outdated
Review
Copy Link

"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.
* Pass new name string when its edited otherwise pass empty string. */
Sybren A. Stüvel commented 2023-05-02 11:11:34 +02:00
Outdated
Review
Copy Link

"its" → "it's been"

"its" → "it's been"
void BKE_fmodifier_name_set(struct FModifier *fcm, const char *name);
/**

5
source/blender/blenkernel/intern/fcurve.c
View File

@ -160,10 +160,11 @@ void BKE_fcurves_copy(ListBase *dst, ListBase *src)
void BKE_fmodifier_name_set(FModifier *fcm, const char *name)
{
/* Copy Modifier name. */
/* Copy new Modifier name. */
BLI_strncpy(fcm->name, name, sizeof(fcm->name));
/* Check unique name. */
/* Set default modifier name when name parameter is an empty string.
* Ensure the name is unique. */
const FModifierTypeInfo *fmi = get_fmodifier_typeinfo(fcm->type);
ListBase list = BLI_listbase_from_link((Link *)fcm);
BLI_uniquename(&list, fcm, fmi->name, '.', offsetof(FModifier, name), sizeof(fcm->name));

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

@ -1109,8 +1109,8 @@ FModifier *add_fmodifier(ListBase *modifiers, int type, FCurve *owner_fcu)
fcm->influence = 1.0f;
BLI_addtail(modifiers, fcm);
/* Set modifier name. */
BKE_fmodifier_name_set(fcm, fmi->name);
/* Set modifier name and make sure it is unique. */
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.
BKE_fmodifier_name_set(fcm, "");
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?

6
source/blender/blenloader/intern/versioning_300.cc
View File

@ -4283,8 +4283,10 @@ void blo_do_versions_300(FileData *fd, Library * /*lib*/, Main *bmain)
}
}
/* Set fcurve modifier name and check their uniqueness when opening old files. Otherwise
* modifiers would have empty name field. */
/* fcm->name was never used to store modifier name so it has always been an empty string. Now
* this property supports name editing. So assign value to name variable of Fmodifier otherwise
Sybren A. Stüvel commented 2023-05-01 16:21:14 +02:00
Outdated
Review
Copy Link

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.
Pratik Borhade commented 2023-05-02 11:09:49 +02:00
Outdated
Review
Copy Link

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).
* modifier interface would show an empty name field. Also ensure uniqueness when opening old
* files. */
if (!MAIN_VERSION_ATLEAST(bmain, 306, 6)) {
LISTBASE_FOREACH (bAction *, act, &bmain->actions) {
LISTBASE_FOREACH (FCurve *, fcu, &act->curves) {