Brushes' use of object mode for polling is ambiguous #119618

New Issue

Lukas Tönne · 2024-03-18T11:17:12+01:00

Lukas Tönne commented

2024-03-18 11:17:12 +01:00

Brush DNA data blocks have ob_mode setting that ties them to a particular object mode. This is used in paint_toolslots_init to filter and select an appropriate brush for the current mode.

This works fine when the brushes are used only for one object type, as is the case for most current modes. For Grease Pencil v3, however, the plan is to use the existing sculpt and paint mode instead of adding a specific mode for GreasePencil objects. The sculpt mode in particular requires special settings from the Brush::gpencil_settings. Checking only the ob_mode of the brush makes it so all brushes with the SCULPT object mode are now eligible for Grease Pencil v3 sculpt mode, even if they don't have the appropriate gpencil_settings.

The paint_toolslots_init can arbitrarily select a default brush that is not suitable for sculpt mode with GPv3 objects. Checking this additional condition would require context-dependent object properties. Using object mode to communicate which type of brush is expected is not enough to distinguish brushes (code).

Brush DNA data blocks have `ob_mode` setting that ties them to a particular object mode. This is used in `paint_toolslots_init` to filter and select an appropriate brush for the current mode. This works fine when the brushes are used only for one object type, as is the case for most current modes. For Grease Pencil v3, however, the plan is to use the existing sculpt and paint mode instead of adding a specific mode for GreasePencil objects. The sculpt mode in particular requires special settings from the [Brush::gpencil_settings](https://projects.blender.org/blender/blender/src/commit/433d91fca8a8e9f821f7f6690974b5b98156c85b/source/blender/makesdna/DNA_brush_types.h#L402). Checking only the `ob_mode` of the brush makes it so _all_ brushes with the `SCULPT` object mode are now eligible for Grease Pencil v3 sculpt mode, even if they don't have the appropriate `gpencil_settings`. The `paint_toolslots_init` can arbitrarily select a default brush that is not suitable for sculpt mode with GPv3 objects. Checking this additional condition would require context-dependent object properties. Using object mode to communicate which type of brush is expected is not enough to distinguish brushes ([code](https://projects.blender.org/blender/blender/src/commit/433d91fca8a8e9f821f7f6690974b5b98156c85b/source/blender/blenkernel/intern/paint.cc#L694)).

Lukas Tönne added the

Type

To Do

label 2024-03-18 11:17:12 +01:00

Lukas Tönne added this to the Grease Pencil project 2024-03-18 11:17:15 +01:00

Lukas Tönne commented

2024-03-18 11:29:00 +01:00

What i would suggest:

Instead of using object mode as proxy for the tool type/paint settings, use maybe the context mode or some other enum that is more specific than the object mode.

The paint->runtime.ob_mode is set in BKE_paint_runtime_init based on the paint type used.

  if (paint == &ts->imapaint.paint) {
    paint->runtime.tool_offset = offsetof(Brush, imagepaint_tool);
    paint->runtime.ob_mode = OB_MODE_TEXTURE_PAINT;
  }
  else if (ts->sculpt && paint == &ts->sculpt->paint) {
    paint->runtime.tool_offset = offsetof(Brush, sculpt_tool);
    paint->runtime.ob_mode = OB_MODE_SCULPT;
  }
  ...

Then BKE_paint_toolslots_brush_validate decodes this into matching brush types.

    if (brush->ob_mode & eObjectMode(paint->runtime.ob_mode)) {
      ...
    }

There are now fewer object modes than there are possible Paint settings and some paint types become indistinguishable to brush poll functions.

What i would suggest: Instead of using object mode as _proxy_ for the tool type/paint settings, use maybe the [context mode](https://projects.blender.org/blender/blender/src/commit/433d91fca8a8e9f821f7f6690974b5b98156c85b/source/blender/blenkernel/BKE_context.hh#L119) or some other enum that is more specific than the object mode. The `paint->runtime.ob_mode` is set in `BKE_paint_runtime_init` based on the paint type used. ```cpp if (paint == &ts->imapaint.paint) { paint->runtime.tool_offset = offsetof(Brush, imagepaint_tool); paint->runtime.ob_mode = OB_MODE_TEXTURE_PAINT; } else if (ts->sculpt && paint == &ts->sculpt->paint) { paint->runtime.tool_offset = offsetof(Brush, sculpt_tool); paint->runtime.ob_mode = OB_MODE_SCULPT; } ... ``` Then `BKE_paint_toolslots_brush_validate` decodes this into matching brush types. ```cpp if (brush->ob_mode & eObjectMode(paint->runtime.ob_mode)) { ... } ``` There are now fewer object modes than there are possible `Paint` settings and some paint types become indistinguishable to brush poll functions.

Brecht Van Lommel commented

2024-03-18 11:32:20 +01:00

For Grease Pencil v3, however, the plan is to use the existing sculpt and paint mode instead of adding a specific mode for GreasePencil objects.

I would not do this change, it sounds like it's going to cause problems. For GPv3 you should really try to change as little other design as possible, and just get things working as they were before. Don't make these kinds of design changes that cause the GPv3 project to take even longer.

> For Grease Pencil v3, however, the plan is to use the existing sculpt and paint mode instead of adding a specific mode for GreasePencil objects. I would not do this change, it sounds like it's going to cause problems. For GPv3 you should really try to change as little other design as possible, and just get things working as they were before. Don't make these kinds of design changes that cause the GPv3 project to take even longer.

Falk David commented

2024-03-18 11:43:01 +01:00

@brecht Would be good to understand why you think this will cause more issues. We're also already using OB_MODE_EDIT and OB_MODE_WEIGHT_PAINT so those would have to be reverted as well.

I'd like to add that one of the goals of GPv3 was to make Grease Pencil a "first class citizen" and not seperate it from the other objects. E.g. we're using the same ModifierData struct for modifiers.

@brecht Would be good to understand why you think this will cause more issues. We're also already using `OB_MODE_EDIT` and `OB_MODE_WEIGHT_PAINT` so those would have to be reverted as well. I'd like to add that one of the goals of GPv3 was to make Grease Pencil a "first class citizen" and not seperate it from the other objects. E.g. we're using the same `ModifierData` struct for modifiers.

Brecht Van Lommel commented

2024-03-18 12:10:01 +01:00

One part of this is a planning thing. If you encounter something like this that requires changing some deeper design, you should consider not doing it, even if you consider the existing design suboptimal. I think it's a big part of the reason the GPv3 project is taking longer than was initially expected, refactoring more logic than needed rather than mechanically changing one data structure to another. I don't think this even needed any new mode bits, the existing ones could have been used.

Regarding the design, I don't think using the same object mode bit is what makes it a first class citizen. Curves sculpt mode also uses its own bit, as brushes are not shared with regular sculpt mode. It's only the same thing in name and UI organization.

The existing brush and asset code relies on them being classified by mode. So I don't see the point in refactoring this and having to solve the design problem this issue is about. I don't think OB_MODE_EDIT has an equivalent problem, so if it works there is no need to change. If OB_MODE_WEIGHT_PAINT has a similar problem already without a solution, maybe it should be changed back.

One part of this is a planning thing. If you encounter something like this that requires changing some deeper design, you should consider not doing it, even if you consider the existing design suboptimal. I think it's a big part of the reason the GPv3 project is taking longer than was initially expected, refactoring more logic than needed rather than mechanically changing one data structure to another. I don't think this even needed any new mode bits, the existing ones could have been used. Regarding the design, I don't think using the same object mode bit is what makes it a first class citizen. Curves sculpt mode also uses its own bit, as brushes are not shared with regular sculpt mode. It's only the same thing in name and UI organization. The existing brush and asset code relies on them being classified by mode. So I don't see the point in refactoring this and having to solve the design problem this issue is about. I don't think `OB_MODE_EDIT` has an equivalent problem, so if it works there is no need to change. If `OB_MODE_WEIGHT_PAINT` has a similar problem already without a solution, maybe it should be changed back.

Falk David commented

2024-03-25 12:33:30 +01:00

We'll go back to OB_MODE_SCULPT_GPENCIL_LEGACY and OB_MODE_WEIGHT_GPENCIL_LEGACY. Closing.

We'll go back to `OB_MODE_SCULPT_GPENCIL_LEGACY` and `OB_MODE_WEIGHT_GPENCIL_LEGACY`. Closing.

Falk David closed this issue

2024-03-25 12:33:33 +01:00

Blender Bot added the

Status

Archived

label 2024-03-25 12:33:35 +01:00

Sign in to join this conversation.