Code Quality: Renaming on BKE_material.h #73589

New Issue

Antonio Vazquez · 2020-02-04T16:08:11+01:00

Antonio Vazquez commented

2020-02-04 16:08:11 +01:00

Rename BKE functions to include BKE_material_prefix.

Proposed changes:

    # Functions (make it clear we're in edit-mode)

("init_def_material", "BKE_materials_init"),
("BKE_material_gpencil_default_free", "BKE_materials_exit"),
("test_object_materials", "BKE_object_materials_test"),
("test_all_objects_materials", "BKE_objects_materials_test_all"),

    # give_node_material                 <<<<< REMOVE (UNUSED) ??????

("give_matarar", "BKE_object_material_array"),
("give_totcolp", "BKE_object_material_num"),
("give_current_material_p", "BKE_object_material_get_p"),
("give_current_material", "BKE_object_material_get"),
("assign_material", "BKE_object_material_assign"),
("assign_matarar", "BKE_object_material_array_assign"),
("give_matarar_id", "BKE_id_material_array"),
("give_totcolp_id", "BKE_id_material_num"),
("assign_material_id", "BKE_id_material_assign"),
("clear_matcopybuf", "BKE_material_copybuf_clear"),
("free_matcopybuf", "BKE_material_copybuf_free"),
("copy_matcopybuf", "BKE_material_copybuf_copy"),
("paste_matcopybuf", "BKE_material_copybuf_paste"),
("BKE_material_init_gpencil_settings", "BKE_gpencil_material_attr_init"),
("BKE_material_add_gpencil", "BKE_gpencil_material_add"),
("BKE_material_gpencil_get", "BKE_gpencil_material"),
("BKE_material_gpencil_default_get", "BKE_gpencil_material_dft"),
("BKE_material_gpencil_settings_get", "BKE_gpencil_material_attr"),

Rename BKE functions to include `BKE_material_`prefix. Proposed changes: ```replace_material = ( # Functions (make it clear we're in edit-mode) ``` ("init_def_material", "BKE_materials_init"), ("BKE_material_gpencil_default_free", "BKE_materials_exit"), ("test_object_materials", "BKE_object_materials_test"), ("test_all_objects_materials", "BKE_objects_materials_test_all"), ``` # give_node_material <<<<< REMOVE (UNUSED) ?????? ``` ("give_matarar", "BKE_object_material_array"), ("give_totcolp", "BKE_object_material_num"), ("give_current_material_p", "BKE_object_material_get_p"), ("give_current_material", "BKE_object_material_get"), ("assign_material", "BKE_object_material_assign"), ("assign_matarar", "BKE_object_material_array_assign"), ("give_matarar_id", "BKE_id_material_array"), ("give_totcolp_id", "BKE_id_material_num"), ("assign_material_id", "BKE_id_material_assign"), ("clear_matcopybuf", "BKE_material_copybuf_clear"), ("free_matcopybuf", "BKE_material_copybuf_free"), ("copy_matcopybuf", "BKE_material_copybuf_copy"), ("paste_matcopybuf", "BKE_material_copybuf_paste"), ("BKE_material_init_gpencil_settings", "BKE_gpencil_material_attr_init"), ("BKE_material_add_gpencil", "BKE_gpencil_material_add"), ("BKE_material_gpencil_get", "BKE_gpencil_material"), ("BKE_material_gpencil_default_get", "BKE_gpencil_material_dft"), ("BKE_material_gpencil_settings_get", "BKE_gpencil_material_attr"), ``` ) ```

Antonio Vazquez self-assigned this 2020-02-04 16:08:12 +01:00

Antonio Vazquez commented

2020-02-04 16:08:12 +01:00

Changed status from 'Needs Triage' to: 'Confirmed'

Antonio Vazquez commented

2020-02-04 16:08:12 +01:00

Added subscribers: @antoniov, @dfelinto

Antonio Vazquez commented

2020-02-04 16:23:03 +01:00

Added subscriber: @brecht

Antonio Vazquez commented

2020-02-04 16:23:03 +01:00

@dfelinto @brecht before doing any renaming, I would like to have some feedback.

Antonio Vazquez commented

2020-02-04 16:26:00 +01:00

Added subscriber: @ideasman42

Antonio Vazquez commented

2020-02-04 16:26:00 +01:00

@ideasman42 I would like to hear your opinions too.

Brecht Van Lommel commented

2020-02-04 16:36:13 +01:00

This is a good but we can do much more here, for example:

init_def_material -> BKE_materials_init, similar to how we init other modules.
BKE_material_node_get: this should actually be removed, only made sense with old Blender material nodes.
give_matarar -> BKE_material_array_from_object
give_matarar_id -> BKE_material_array_from_ID
give_totcolp -> BKE_material_num_from_object (return value instead of pointer, and refactor code as needed)
give_totcolp_id -> BKE_material_num_from_ID
give_current_material -> BKE_material_array_get

This is a good but we can do much more here, for example: * `init_def_material` -> `BKE_materials_init`, similar to how we init other modules. * `BKE_material_node_get`: this should actually be removed, only made sense with old Blender material nodes. * `give_matarar` -> `BKE_material_array_from_object` * `give_matarar_id` -> `BKE_material_array_from_ID` * `give_totcolp` -> `BKE_material_num_from_object` (return value instead of pointer, and refactor code as needed) * `give_totcolp_id` -> `BKE_material_num_from_ID` * `give_current_material` -> `BKE_material_array_get`

Antonio Vazquez commented

2020-02-04 16:51:18 +01:00

@brecht I have seen these in anim_filter.c:

if (has_nested == false) {
  has_nested = (give_node_material(ma) != NULL);
}```


```if (has_nested) {

for (a = 1; a <= ob->totcol; a++) {
Material *base = give_current_material(ob, a);
Material *ma = give_node_material(base);```

@brecht I have seen these in `anim_filter.c`: ``` /* for optimising second pass - check if there's a nested material here to come back for */ ``` if (has_nested == false) { has_nested = (give_node_material(ma) != NULL); }``` ``` ```if (has_nested) { ``` for (a = 1; a <= ob->totcol; a++) { Material *base = give_current_material(ob, a); Material *ma = give_node_material(base);```

Brecht Van Lommel commented

2020-02-04 16:55:02 +01:00

Yes, that code can be removed, nested materials do not exist anymore.

Antonio Vazquez commented

2020-02-04 18:16:46 +01:00

Renaming patch in D6751

Renaming patch in [D6751](https://archive.blender.org/developer/D6751)

Campbell Barton commented

2020-02-05 02:42:23 +01:00

Prefer if batch renaming is handled with a script to more easily apply changes & update names eg: P1241

Some open topics:

Should we use the term 'active' instead of current?

This is more common in Blender, even variable names in C/RNA call this active.
For API calls it's often not clear if this operates on a material, and ID or an object, with the addition of grease pencil we could accept longer names and include object/ID/gpencil in all API calls that don't operate directly on Materials?
BKE_material_get too fuzzy, also naming is different from BKE_material_current_p_get when they're equivalent functions with only a minor difference, suggest:
- BKE_material_object_active_get
- BKE_material_object_active_get_p

... then rename:

BKE_material_gpencil_get to BKE_material_gpencil_active_get

Could use BKE_material_active_from_object style naming, however it doesn't group get/set functions so neatly.

Prefer if batch renaming is handled with a script to more easily apply changes & update names eg: [P1241](https://archive.blender.org/developer/P1241.txt) Some open topics: - Should we use the term 'active' instead of current? *This is more common in Blender, even variable names in C/RNA call this active.* - For API calls it's often not clear if this operates on a material, and ID or an object, with the addition of grease pencil we could accept longer names and include `object/ID/gpencil` in all API calls that don't operate directly on Materials? - `BKE_material_get` too fuzzy, also naming is different from `BKE_material_current_p_get` when they're equivalent functions with only a minor difference, suggest: - `BKE_material_object_active_get` - `BKE_material_object_active_get_p` ``` ... then rename: ``` - `BKE_material_gpencil_get` to `BKE_material_gpencil_active_get` *Could use `BKE_material_active_from_object` style naming, however it doesn't group get/set functions so neatly.*

Brecht Van Lommel commented

2020-02-05 07:51:49 +01:00

BKE_material_object_active_get

This is still not correct to me, it gets the material at a specified index, and that parameter happens to be named "act". But that isn't necessarily the active material. I suggest BKE_object_material_get. There are already other functions with BKE_object_material prefix in this file.

BKE_material_object_active_get_p

This function is used in just one place to test if the material slot exists. So rather than this hacky returning of pointers to pointers we should really just have a BKE_object_material_slot_exists() for that case and remove this from the API.

> BKE_material_object_active_get This is still not correct to me, it gets the material at a specified index, and that parameter happens to be named "act". But that isn't necessarily the active material. I suggest `BKE_object_material_get`. There are already other functions with `BKE_object_material` prefix in this file. > BKE_material_object_active_get_p This function is used in just one place to test if the material slot exists. So rather than this hacky returning of pointers to pointers we should really just have a `BKE_object_material_slot_exists()` for that case and remove this from the API.

Campbell Barton commented

2020-02-05 08:12:32 +01:00

Tsk, my mistake give_current_material reads like get active material, of course it's accessing via index (index + 1 in fact, better handle that separate from rename).

+1 for using BKE_object_material_ assume this applies to BKE_gpencil_material_, BKE_id_material_ too.

Tsk, my mistake `give_current_material` reads like `get active material`, of course it's accessing via index (`index + 1` in fact, better handle that separate from rename). +1 for using `BKE_object_material_` assume this applies to `BKE_gpencil_material_`, `BKE_id_material_` too.

Brecht Van Lommel commented

2020-02-05 08:30:17 +01:00

@ideasman42, we linked to VS Code refactoring tools here, which has Rename Symbol.
https://wiki.blender.org/wiki/Style_Guide/Code_Quality_Day

I think that is fine as long as the developer mentions in the patch which header file to look at for changes, and that the rest is just automated renaming.

Then it's also possible to reorder functions, group functions, comment, rename parameters, etc, to make it a consistent whole. That isn't a requirement for these kinds of tasks, but when it's done it takes some iteration and isn't easy to separate from the renaming.

@ideasman42, we linked to VS Code refactoring tools here, which has Rename Symbol. https://wiki.blender.org/wiki/Style_Guide/Code_Quality_Day I think that is fine as long as the developer mentions in the patch which header file to look at for changes, and that the rest is just automated renaming. Then it's also possible to reorder functions, group functions, comment, rename parameters, etc, to make it a consistent whole. That isn't a requirement for these kinds of tasks, but when it's done it takes some iteration and isn't easy to separate from the renaming.

Campbell Barton commented

2020-02-05 09:00:04 +01:00

For small/medium refactors which don't require much review/iteration, I'm not so fussed.

When changing many symbols in an API, it's useful to see the result in context, see how the changes fit with whats already there,
sometimes you notice inconsistencies or odd conventions that feed back into better decisions, eg: D2678

For small/medium refactors which don't require much review/iteration, I'm not so fussed. When changing many symbols in an API, it's useful to see the result in context, see how the changes fit with whats already there, sometimes you notice inconsistencies or odd conventions that feed back into better decisions, eg: [D2678](https://archive.blender.org/developer/D2678)

Antonio Vazquez commented

2020-02-05 09:40:42 +01:00

I like more the script solution than VS refactor tools for renaming. I use Visual Studio 2019 (big brother of VS) and sometimes the refactor miss code and is very slow if you compare with find/replace. In my job in other languages (not blender) I always have used find/replace scripts with very good results and speed.

Only one comment about the script, it would be better to add ( at the end of the original and destination strings ("init_def_material" -> "init_def_material(") to avoid naming conflicts. It could be any situation where one renaming could replace a section of other naming, and the (avoids that.

If you agree, I can prepare the script and we can verify naming again before run it. IMHO, any refactor of parameters, returns, etc, must be in a different task.

EDIT: It looks using ( at the end of the string breaks the script, so I'm going to remove it.

I like more the script solution than VS refactor tools for renaming. I use Visual Studio 2019 (big brother of VS) and sometimes the refactor miss code and is very slow if you compare with find/replace. In my job in other languages (not blender) I always have used find/replace scripts with very good results and speed. Only one comment about the script, it would be better to add `(` at the end of the original and destination strings (`"init_def_material"` -> `"init_def_material("`) to avoid naming conflicts. It could be any situation where one renaming could replace a section of other naming, and the `(`avoids that. If you agree, I can prepare the script and we can verify naming again before run it. IMHO, any refactor of parameters, returns, etc, must be in a different task. EDIT: It looks using `(` at the end of the string breaks the script, so I'm going to remove it.

Antonio Vazquez commented

2020-02-05 09:54:26 +01:00

replace_material = (
    # Functions (make it clear we're in edit-mode)
    ("init_def_material",                  "BKE_materials_init"),
    ("test_object_materials",              "BKE_material_object_test"),
    ("test_all_objects_materials",         "BKE_material_object_test_all"),
    # give_node_material                 <<<<< REMOVE (UNUSED) ??????
    ("give_matarar",                       "BKE_material_array_from_object"),
    ("give_totcolp",                       "BKE_material_num_from_object"),
    ("give_matarar_id",                    "BKE_material_array_from_ID"),
    ("give_totcolp_id",                    "BKE_material_num_from_ID"),
    ("give_current_material_p",            "BKE_object_material_p"),
    ("give_current_material",              "BKE_object_material"),
    ("assign_material_id",                 "BKE_material_assign_ID"),
    ("assign_material",                    "BKE_material_assign"),
    ("assign_matarar",                     "BKE_material_assign_array"),
    ("clear_matcopybuf",                   "BKE_material_copybuf_clear"),
    ("free_matcopybuf",                    "BKE_material_copybuf_free"),
    ("copy_matcopybuf",                    "BKE_material_copybuf_copy"),
    ("paste_matcopybuf",                   "BKE_material_copybuf_paste"),
    ("BKE_material_init_gpencil_settings", "BKE_gpencil_material_attr_init"),
    ("BKE_material_add_gpencil",           "BKE_gpencil_material_add"),
    ("BKE_material_gpencil_get",           "BKE_gpencil_material"),
    ("BKE_material_gpencil_default_get",   "BKE_gpencil_material_dft"),
    ("BKE_material_gpencil_settings_get",  "BKE_gpencil_material_attr"),
)```

``` replace_material = ( # Functions (make it clear we're in edit-mode) ("init_def_material", "BKE_materials_init"), ("test_object_materials", "BKE_material_object_test"), ("test_all_objects_materials", "BKE_material_object_test_all"), # give_node_material <<<<< REMOVE (UNUSED) ?????? ("give_matarar", "BKE_material_array_from_object"), ("give_totcolp", "BKE_material_num_from_object"), ("give_matarar_id", "BKE_material_array_from_ID"), ("give_totcolp_id", "BKE_material_num_from_ID"), ("give_current_material_p", "BKE_object_material_p"), ("give_current_material", "BKE_object_material"), ("assign_material_id", "BKE_material_assign_ID"), ("assign_material", "BKE_material_assign"), ("assign_matarar", "BKE_material_assign_array"), ("clear_matcopybuf", "BKE_material_copybuf_clear"), ("free_matcopybuf", "BKE_material_copybuf_free"), ("copy_matcopybuf", "BKE_material_copybuf_copy"), ("paste_matcopybuf", "BKE_material_copybuf_paste"), ("BKE_material_init_gpencil_settings", "BKE_gpencil_material_attr_init"), ("BKE_material_add_gpencil", "BKE_gpencil_material_add"), ("BKE_material_gpencil_get", "BKE_gpencil_material"), ("BKE_material_gpencil_default_get", "BKE_gpencil_material_dft"), ("BKE_material_gpencil_settings_get", "BKE_gpencil_material_attr"), )```

Antonio Vazquez commented

2020-02-05 10:13:19 +01:00

I have tested the script and in my PC takes a few seconds to run.

Brecht Van Lommel commented

2020-02-05 10:30:02 +01:00

I'm fine with using scripts when it works well.

If we follow the BKE_object_material and BKE_id_material prefix then it would look like this. I also added BKE_materials_exit for consistency with init.

replace_material = (
    # Functions (make it clear we're in edit-mode)
    ("init_def_material",                  "BKE_materials_init"),
    ("BKE_material_gpencil_default_free",  "BKE_materials_exit"),
    ("test_object_materials",              "BKE_object_materials_test"),
    ("test_all_objects_materials",         "BKE_objects_materials_test_all"),
    # give_node_material                 <<<<< REMOVE (UNUSED) ??????
    ("give_matarar",                       "BKE_object_material_array"),
    ("give_totcolp",                       "BKE_object_material_num"),
    ("give_current_material_p",            "BKE_object_material_get_p"),
    ("give_current_material",              "BKE_object_material_get"),
    ("assign_material",                    "BKE_object_material_assign"),
    ("assign_matarar",                     "BKE_object_material_array_assign"),
    ("give_matarar_id",                    "BKE_id_material_array"),
    ("give_totcolp_id",                    "BKE_id_material_num"),
    ("assign_material_id",                 "BKE_id_material_assign"),
    ("clear_matcopybuf",                   "BKE_material_copybuf_clear"),
    ("free_matcopybuf",                    "BKE_material_copybuf_free"),
    ("copy_matcopybuf",                    "BKE_material_copybuf_copy"),
    ("paste_matcopybuf",                   "BKE_material_copybuf_paste"),
    ("BKE_material_init_gpencil_settings", "BKE_gpencil_material_attr_init"),
    ("BKE_material_add_gpencil",           "BKE_gpencil_material_add"),
    ("BKE_material_gpencil_get",           "BKE_gpencil_material"),
    ("BKE_material_gpencil_default_get",   "BKE_gpencil_material_dft"),
    ("BKE_material_gpencil_settings_get",  "BKE_gpencil_material_attr"),
)

I'm fine with using scripts when it works well. If we follow the `BKE_object_material` and `BKE_id_material` prefix then it would look like this. I also added `BKE_materials_exit` for consistency with `init`. ``` replace_material = ( # Functions (make it clear we're in edit-mode) ("init_def_material", "BKE_materials_init"), ("BKE_material_gpencil_default_free", "BKE_materials_exit"), ("test_object_materials", "BKE_object_materials_test"), ("test_all_objects_materials", "BKE_objects_materials_test_all"), # give_node_material <<<<< REMOVE (UNUSED) ?????? ("give_matarar", "BKE_object_material_array"), ("give_totcolp", "BKE_object_material_num"), ("give_current_material_p", "BKE_object_material_get_p"), ("give_current_material", "BKE_object_material_get"), ("assign_material", "BKE_object_material_assign"), ("assign_matarar", "BKE_object_material_array_assign"), ("give_matarar_id", "BKE_id_material_array"), ("give_totcolp_id", "BKE_id_material_num"), ("assign_material_id", "BKE_id_material_assign"), ("clear_matcopybuf", "BKE_material_copybuf_clear"), ("free_matcopybuf", "BKE_material_copybuf_free"), ("copy_matcopybuf", "BKE_material_copybuf_copy"), ("paste_matcopybuf", "BKE_material_copybuf_paste"), ("BKE_material_init_gpencil_settings", "BKE_gpencil_material_attr_init"), ("BKE_material_add_gpencil", "BKE_gpencil_material_add"), ("BKE_material_gpencil_get", "BKE_gpencil_material"), ("BKE_material_gpencil_default_get", "BKE_gpencil_material_dft"), ("BKE_material_gpencil_settings_get", "BKE_gpencil_material_attr"), ) ```

Antonio Vazquez commented

2020-02-05 11:09:43 +01:00

If both agree, I can commit the changes this afternoon.

Brecht Van Lommel commented

2020-02-05 11:12:05 +01:00

It looks good to me now, except I wouldn't use obscure abbrevations for these but instead:

    ("BKE_material_gpencil_default_get",   "BKE_gpencil_material_default"),
    ("BKE_material_gpencil_settings_get",  "BKE_gpencil_material_settings"),

It looks good to me now, except I wouldn't use obscure abbrevations for these but instead: ``` ("BKE_material_gpencil_default_get", "BKE_gpencil_material_default"), ("BKE_material_gpencil_settings_get", "BKE_gpencil_material_settings"), ```

Antonio Vazquez commented

2020-02-05 11:17:25 +01:00

ok, changed, I will do the commit today.

Antonio Vazquez commented

2020-02-05 16:01:00 +01:00

Changed status from 'Confirmed' to: 'Resolved'

Antonio Vazquez closed this issue

2020-02-05 16:01:00 +01:00

Campbell Barton commented

2020-02-06 02:21:36 +01:00

Would rather this patch have gone over more review, there are still many inconsistencies in the API naming.

BKE_material_ prefix for object API's (eg BKE_material_resize_object).
BKE_gpencil_material_attr_init and BKE_material_gpencil_init are similar functions, named differently.
BKE_gpencil_material is a wrapper for BKE_object_material_get which uses a gpencil fallback, should be named more clearly.

Submitted #73623 (Code Quality: Renaming on BKE_material.h #2) to continue this task.

Would rather this patch have gone over more review, there are still many inconsistencies in the API naming. - `BKE_material_` prefix for object API's (eg `BKE_material_resize_object`). - `BKE_gpencil_material_attr_init` and `BKE_material_gpencil_init` are similar functions, named differently. - `BKE_gpencil_material` is a wrapper for `BKE_object_material_get` which uses a gpencil fallback, should be named more clearly. Submitted #73623 (Code Quality: Renaming on BKE_material.h #2) to continue this task.

Campbell Barton commented

2020-02-06 02:27:53 +01:00

In #73589#865691, @antoniov wrote:
Only one comment about the script, it would be better to add ( at the end of the original and destination strings ("init_def_material" -> "init_def_material(") to avoid naming conflicts. It could be any situation where one renaming could replace a section of other naming, and the (avoids that.

This isn't needed as the replacement uses regular-expressions, surrounding the identifier with \b to delimit on word boundaries.

> In #73589#865691, @antoniov wrote: > Only one comment about the script, it would be better to add `(` at the end of the original and destination strings (`"init_def_material"` -> `"init_def_material("`) to avoid naming conflicts. It could be any situation where one renaming could replace a section of other naming, and the `(`avoids that. This isn't needed as the replacement uses regular-expressions, surrounding the identifier with `\b` to delimit on word boundaries.

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Code Quality: Renaming on BKE_material.h #73589