Refactor: Sculpt/Paint: Rename brush "tool" to "brush type" #126796

Julian Eisel · 2024-08-26T18:42:26+02:00

Julian Eisel commented

2024-08-26 18:42:26 +02:00

The term "tool" is historic from before the actual tool system got
introduced. Since then the term was already a bit confusing, because it
wasn't directly related to the tool system, but there was still some
relationship between the two. Now brushes and their types are decoupled
much more from the tool system, with a single "Brush" tool supporting
all kinds of brushes (draw, grab, cloth, smooth, ...).

For a more clear terminology, use "brush type" instead of "tool".

For #126032 we need to write the brush type to the asset metadata (done
in !124618), so we can filter brushes based on the type (so the grease
pencil eraser tool only shows eraser brushes, for example). I'd like to
use future proof names for that to avoid versioning of asset metadata in
future, so I'd rather do the full naming change now.

RNA properties (thus BPY names) are not changed for compatibility
reasons. Can be done in 5.0, see blender/blender#124201.

Old name	New name
`Brush.sculpt_tool`	`Brush.sculpt_brush_type`
`Brush.vertexpaint_tool`	`Brush.vertex_brush_type`
`Brush.weightpaint_tool`	`Brush.weight_brush_type`
`Brush.imagepaint_tool`	`Brush.image_brush_type`
`Brush.gpencil_tool`	`Brush.gpencil_brush_type`
`Brush.gpencil_vertex_tool`	`Brush.gpencil_vertex_brush_type`
`Brush.gpencil_sculpt_tool`	`Brush.gpencil_sculpt_brush_type`
`Brush.gpencil_weight_tool`	`Brush.gpencil_weight_brush_type`
`Brush.curves_sculpt_tool`	`Brush.curves_sculpt_brush_type`

The term "tool" is historic from before the actual tool system got introduced. Since then the term was already a bit confusing, because it wasn't directly related to the tool system, but there was still some relationship between the two. Now brushes and their types are decoupled much more from the tool system, with a single "Brush" tool supporting all kinds of brushes (draw, grab, cloth, smooth, ...). For a more clear terminology, use "brush type" instead of "tool". For #126032 we need to write the brush type to the asset metadata (done in !124618), so we can filter brushes based on the type (so the grease pencil eraser tool only shows eraser brushes, for example). I'd like to use future proof names for that to avoid versioning of asset metadata in future, so I'd rather do the full naming change now. RNA properties (thus BPY names) are not changed for compatibility reasons. Can be done in 5.0, see blender/blender#124201. | Old name | New name | | --- | --- | | `Brush.sculpt_tool` | `Brush.sculpt_brush_type` | | `Brush.vertexpaint_tool` | `Brush.vertex_brush_type` | | `Brush.weightpaint_tool` | `Brush.weight_brush_type` | | `Brush.imagepaint_tool` | `Brush.image_brush_type` | | `Brush.gpencil_tool` | `Brush.gpencil_brush_type` | | `Brush.gpencil_vertex_tool` | `Brush.gpencil_vertex_brush_type` | | `Brush.gpencil_sculpt_tool` | `Brush.gpencil_sculpt_brush_type` | | `Brush.gpencil_weight_tool` | `Brush.gpencil_weight_brush_type` | | `Brush.curves_sculpt_tool` | `Brush.curves_sculpt_brush_type` |

👍 1

Julian Eisel added the

Module

Sculpt, Paint & Texture

label 2024-08-26 18:42:26 +02:00

Julian Eisel added 1 commit 2024-08-26 18:42:37 +02:00

Refactor: Sculpt/Paint: Rename brush "tool" to brush "type" 94b57fba47

The term "tool" is historic from before the actual tool system got
introduced. Since then the term was already a bit confusing, because it
wasn't directly related to the tool system, but there was still some
relationship between the two. Now brushes and their types are decoupled
much more from the tool system, with a single "Brush" tool supporting
all kinds of brushes (draw, grab, cloth, smooth, ...).

For a much more clear terminology, use "brush type" instead of "tool".

For #126032 we need to write the brush type to the asset metadata (done
in !124618), so we can filter brushes based on the type (so the grease
pencil eraser tool only shows eraser brushes, for example). I'd like to
use future proof names for that to avoid versioning in future, so I'd
rather do the full naming change now.

Included changes:
- Add new RNA properties instead of renaming existing ones. These seem
  commonly accessed from BPY so better keep the legacy names available
  and remove them for 5.0, see #124201.
- Rename DNA members, enumerators, enums,
- Update comments, function names, etc.

Julian Eisel added 1 commit 2024-08-26 18:52:24 +02:00

Cleanup: Use new names, not legacy names in Python ea83905b29

Julian Eisel changed title from ~~Refactor: Sculpt/Paint: Rename brush "tool" to brush "type"~~ to Refactor: Sculpt/Paint: Rename brush "tool" to "brush type"

2024-08-26 18:55:13 +02:00

Julian Eisel added 1 commit 2024-08-27 14:41:32 +02:00

Cleanup: Avoid overly verbose name e475a6e9b5

Sergey Sharybin requested changes 2024-08-27 17:16:58 +02:00

Dismissed

Sergey Sharybin left a comment

On the C/C++ side it is a good change.

The RNA part I am not really happy with the current state of PR: other than a comment in C++ there is no way to know that some fields are deprecated.
If we go the route of "soft deprecation" there needs to be some sort of warning generated when you access the field (we had something similar for bgl module, AFAIR).
And, surely, it should be clearly reflected in the API documentation. Unless I'm missing something with the current PR the sculpt_tool and sculpt_brush_type will have the same exact explanation, and people will be confused which one to use.

On the C/C++ side it is a good change. The RNA part I am not really happy with the current state of PR: other than a comment in C++ there is no way to know that some fields are deprecated. If we go the route of "soft deprecation" there needs to be some sort of warning generated when you access the field (we had something similar for bgl module, AFAIR). And, surely, it should be clearly reflected in the API documentation. Unless I'm missing something with the current PR the `sculpt_tool` and `sculpt_brush_type` will have the same exact explanation, and people will be confused which one to use.

Bastien Montagne reviewed 2024-08-27 17:51:31 +02:00

Bastien Montagne left a comment

I agree that we should not expose both old and new names as 'same thing' in the API and its documentation.

A quick hack could be to add accessors to the deprecated properties, generating some CLOG warnings e.g.

But I wonder if it would not be a good opportunity to actually add a nice way to handle RNA deprecation. Something like a new RNA_def_property_deprecated(PropertyRNA *prop, StringRefNull message, uint3 removal_version) function that would set a flag in the property definition, and store the expected removal version.

Then:

This info is available from bpy (Type.bl_rna.properties["deprecated_prop"]), so API doc can either ignore them, and/or add proper deprecated info.
This info is used by makesrna to:
- Generate deprecated warning when property is accessed.
- Assert when current Blender version reaches the 'expected removal' one (which makes it trivial to keep track of deprecated things to remove as scheduled).

This is getting fairly out of scope for this PR, but at some point we do need to improve our deprecation handling.

I agree that we should not expose both old and new names as 'same thing' in the API and its documentation. A quick hack could be to add accessors to the deprecated properties, generating some CLOG warnings e.g. But I wonder if it would not be a good opportunity to actually add a nice way to handle RNA deprecation. Something like a new `RNA_def_property_deprecated(PropertyRNA *prop, StringRefNull message, uint3 removal_version)` function that would set a flag in the property definition, and store the expected removal version. Then: * This info is available from bpy (`Type.bl_rna.properties["deprecated_prop"]`), so API doc can either ignore them, and/or add proper deprecated info. * This info is used by `makesrna` to: - Generate deprecated warning when property is accessed. - Assert when current Blender version reaches the 'expected removal' one (which makes it trivial to keep track of deprecated things to remove as scheduled). This is getting fairly out of scope for this PR, but at some point we do need to improve our deprecation handling.

Sergey Sharybin commented

2024-08-28 10:49:52 +02:00

Thanks Bastien.

For keeping things practical and moving forward, how do you both feel of limiting this PR to C/C++ side, and re-iterating on the Python side later, once deprecation mechanism is implemented?

For the deprecation warning I have some concerns (about implementation details, not the idea that we need to improve/implement it).

People working on scripts are surely more technical than users, but not technical enough to always pay attention to the terminal. On macOS it is not in a spirit to run applications from the terminals. And on Linux default .desktop shortcuts don't run Blender via terminal either. So ideally such warnings are somehow better be exposed to the UI.

On another hand, people who use scripts are not necessarily people maintaining them. For some more technical artists even console logs might become annoying about things they are not controlling. If the warning is done in the UI, then it becomes even more annoying for everyone.

Perhaps something like "warn once per deprecated property access" would be good default. And maybe enabling Developers Extra would default to "warn on every deprecated access".

For the RNA_def_property_deprecated we'd need to add some field that will explain what is to be used instead.

Anyway, just some current thoughts. We probably move further discussion to a devtalk :)

Thanks Bastien. For keeping things practical and moving forward, how do you both feel of limiting this PR to C/C++ side, and re-iterating on the Python side later, once deprecation mechanism is implemented? For the deprecation warning I have some concerns (about implementation details, not the idea that we need to improve/implement it). People working on scripts are surely more technical than users, but not technical enough to always pay attention to the terminal. On macOS it is not in a spirit to run applications from the terminals. And on Linux default .desktop shortcuts don't run Blender via terminal either. So ideally such warnings are somehow better be exposed to the UI. On another hand, people who use scripts are not necessarily people maintaining them. For some more technical artists even console logs might become annoying about things they are not controlling. If the warning is done in the UI, then it becomes even more annoying for everyone. Perhaps something like "warn once per deprecated property access" would be good default. And maybe enabling Developers Extra would default to "warn on every deprecated access". For the `RNA_def_property_deprecated` we'd need to add some field that will explain what is to be used instead. Anyway, just some current thoughts. We probably move further discussion to a devtalk :)

👍 1

Julian Eisel referenced this pull request

2024-08-30 16:58:52 +02:00

Sculpt/paint: Write brush types to brush asset metadata #124618

Julian Eisel referenced this issue from a commit

2024-08-30 17:00:34 +02:00

Sculpt/paint: Write brush types to brush asset metadata

Julian Eisel added 1 commit 2024-08-30 18:18:08 +02:00

Merge branch 'main' into temp-refactor-brush-tool-rename-brush-type 6c1fbcef2b

Julian Eisel added 1 commit 2024-08-30 18:29:04 +02:00

Fix build errors after merge 9ac1cbb431

Julian Eisel added 1 commit 2024-08-30 18:38:16 +02:00

Undo Python API changes f485c9fc3f

Sergey Sharybin commented

2024-09-02 10:49:10 +02:00

@JulianEisel I think description needs to be updated, and there are also merge conflicts.
Is a big change, so only did a quick pass without checking line-by-line. What could possibly go wrong with the simple rename :)

@JulianEisel I think description needs to be updated, and there are also merge conflicts. Is a big change, so only did a quick pass without checking line-by-line. What could possibly go wrong with the simple rename :)

Julian Eisel added 1 commit 2024-09-02 11:26:18 +02:00

Merge branch 'main' into temp-refactor-brush-tool-rename-brush-type

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details