Regression: Ctrl+Z breaks vertex paint #116029

New Issue

Closed

opened 2023-12-11 12:06:47 +01:00 by Full-Name-8 · 11 comments

Full-Name-8 commented

2023-12-11 12:06:47 +01:00

System Information
Operating system: Windows-10-10.0.22631-SP0 64 Bits
Graphics card: NVIDIA GeForce RTX 3060 Ti/PCIe/SSE2 NVIDIA Corporation 4.6.0 NVIDIA 546.17

Blender Version
Broken: version: 4.0.2, branch: blender-v4.0-release, commit date: 2023-12-05 07:41, hash: 9be62e85b727
Worked: 3.6

This changed in 94e6ab6d71

Short description of error
When I press Ctrl+Z several times in vertex paint mode, in some point it returns to object mode. Than when i go in vertex paint and tryin to paint nothing happens, the vertices are not painted no matter what I do. If you create a new mash in the same scene, the vertices are not painted on it either. If I create a new file and Ctrl+V broken object from previous file the vertices are not painted over either.

Exact steps for others to reproduce the error
New file, default cube or any created mash

Switch to the vertex paint mode
Paint any vertex in any color
Press Ctrl+Z (this cancels the previous action)
Press Ctrl+Z again (this switches to the object mode)
Switch to the vertex paint mode
The vertices are no longer painted

**System Information** Operating system: Windows-10-10.0.22631-SP0 64 Bits Graphics card: NVIDIA GeForce RTX 3060 Ti/PCIe/SSE2 NVIDIA Corporation 4.6.0 NVIDIA 546.17 **Blender Version** Broken: version: 4.0.2, branch: blender-v4.0-release, commit date: 2023-12-05 07:41, hash: `9be62e85b727` Worked: 3.6 This changed in 94e6ab6d71 **Short description of error** When I press Ctrl+Z several times in vertex paint mode, in some point it returns to object mode. Than when i go in vertex paint and tryin to paint nothing happens, the vertices are not painted no matter what I do. If you create a new mash in the same scene, the vertices are not painted on it either. If I create a new file and Ctrl+V broken object from previous file the vertices are not painted over either. **Exact steps for others to reproduce the error** New file, default cube or any created mash 1. Switch to the vertex paint mode 2. Paint any vertex in any color 3. Press Ctrl+Z (this cancels the previous action) 4. Press Ctrl+Z again (this switches to the object mode) 5. Switch to the vertex paint mode 6. The vertices are no longer painted

Full-Name-8 added the

labels 2023-12-11 12:06:48 +01:00

Pratik Borhade commented

2023-12-12 10:53:05 +01:00

Member

Hi, thanks for the report. Can confirm. This appears to be broke between 3.6 - 4.0 release

This seems to happen when existing color attribute is deleted during undo

Hi, thanks for the report. Can confirm. This appears to be broke between 3.6 - 4.0 release This seems to happen when existing color attribute is deleted during undo

Pratik Borhade added

Sculpt, Paint & Texture

and removed

labels 2023-12-12 10:53:18 +01:00

Pratik Borhade referenced this issue

2023-12-12 10:55:40 +01:00

Vertex paint mode breaks permanently under certain conditions (Global undo involved) #107845

Pratik Borhade changed title from ~~Ctrl+Z breaks vertex paint~~ to Regression: Ctrl+Z breaks vertex paint

2023-12-12 10:55:57 +01:00

Philipp Oeser referenced this issue

2023-12-12 12:12:02 +01:00

Vertex paint mode breaks permanently under certain conditions (Global undo involved) #107845

Philipp Oeser commented

2023-12-12 12:29:57 +01:00

Member

Broke between good cc5f666672 & bad 9cbe156d6a
Will bisect in a bit...

Broke between good cc5f66667244 & bad 9cbe156d6a93 Will bisect in a bit...

Philipp Oeser commented

2023-12-13 11:24:34 +01:00

Member

This changed in 94e6ab6d71

@mont29 : mind checking?

This changed in 94e6ab6d718735b9646bd11fb3196c5fe224fd74 @mont29 : mind checking?

Philipp Oeser added the

labels 2023-12-13 11:26:02 +01:00

Bastien Montagne commented

2023-12-14 11:15:32 +01:00

Owner

Paint is still working fine, the problem is that the paint settings are reset (brush goes back to 'Add' e.g.).

Think it happened to work before 94e6ab6d71 by mere accident (issue fixed by 94e6ab6d71 hiding the one revealed here).

Checking further.

Paint is still working fine, the problem is that the paint settings are reset (brush goes back to 'Add' e.g.). Think it happened to work before 94e6ab6d71 by mere accident (issue fixed by 94e6ab6d71 hiding the one revealed here). Checking further.

Bastien Montagne commented

2023-12-14 12:15:55 +01:00

Owner

The problem comes from the tool system resetting the active tool (and brush) from the content of the relevant tool_slots. For some reasons, on startup, the first tool slot is set to Add brush (at least for me), instead of matching the default Draw brush.

The problem comes from the tool system resetting the active tool (and brush) from the content of the relevant `tool_slots`. For some reasons, on startup, the first tool slot is set to `Add` brush (at least for me), instead of matching the default `Draw` brush.

Bastien Montagne referenced this issue

2023-12-14 18:01:38 +01:00

WIP 'Magic Fix' for #116029. #116197

Bastien Montagne commented

2023-12-14 18:02:33 +01:00

Owner

I'm lost here...

The active scene's vpaint brush is reset to Add one (i.e. the one in the tool_slots[0]) in the WM_toolsystem_refresh_active call part of the ed_undo_step_post function. This happens when iterating over all tool references in the workspace, and calling toolsystem_refresh_ref on them.

From my understanding, in toolsystem_ref_link, there is a mismatch between the 'active' slot_index, and the 'active' tool stored in the Brush ID itself? And why do we store the active tool in the brush ID????
This function also seems to be the only one calling BKE_brush_tool_set, even though it first uses the result of BKE_brush_tool_get to decide whether it needs to re-assign a brush or not (which is what if causing the Draw brush to be replaced by the Add one here). So how is that 'active tool' returned by BKE_brush_tool_get supposed to be set and valid in the first place?

It seems that the tool_slots used for a brush is defined by its assigned by BKE_paint_brush_set (through call to BKE_paint_toolslots_brush_update_ex).

NOTE: Ended up finding 'something' that 'fixes' the reported issue: !116197 . It seems to be a valid change looking at the code... But I have zero idea whether this double brush setting call is actually needed for something else, nor why it was added in the first place (comes from a90bcdf93d ).

I am not familiar enough with the Tool system to go further am afraid, not without spending days trying to understand it. @ideasman42, mind taking over?

I'm lost here... The active scene's vpaint brush is reset to `Add` one (i.e. the one in the `tool_slots[0]`) in the `WM_toolsystem_refresh_active` call part of the `ed_undo_step_post` function. This happens when iterating over all tool references in the workspace, and calling `toolsystem_refresh_ref` on them. From my understanding, in `toolsystem_ref_link`, there is a mismatch between the 'active' `slot_index`, and the 'active' tool stored in the Brush ID itself? And why do we store the active tool in the brush ID???? This function also seems to be the only one calling `BKE_brush_tool_set`, even though it first uses the result of `BKE_brush_tool_get` to decide whether it needs to re-assign a brush or not (which is what if causing the `Draw` brush to be replaced by the `Add` one here). So how is that 'active tool' returned by `BKE_brush_tool_get` supposed to be set and valid in the first place? It seems that the `tool_slots` used for a brush is defined by its assigned by `BKE_paint_brush_set` (through call to `BKE_paint_toolslots_brush_update_ex`). NOTE: Ended up finding 'something' that 'fixes' the reported issue: !116197 . It _seems_ to be a valid change looking at the code... But I have zero idea whether this double brush setting call is actually needed for something else, nor why it was added in the first place (comes from a90bcdf93d82bf5 ). I am not familiar enough with the Tool system to go further am afraid, not without spending days trying to understand it. @ideasman42, mind taking over?

Campbell Barton was assigned by Bastien Montagne

2023-12-14 18:02:46 +01:00

Bastien Montagne added

and removed

labels 2023-12-14 18:03:03 +01:00

Sergey Sharybin commented

2024-01-11 16:22:59 +01:00

Owner

@ideasman42 Hey. Any news here?

@ideasman42 Hey. Any news here?

Campbell Barton referenced a pull request that will close this issue

2024-01-12 08:37:38 +01:00

WIP: Fix #116029: Undo restores the wrong brush in vertex paint mode #117055

Campbell Barton commented

2024-01-12 08:40:05 +01:00

Owner

Edit these findings look to be flawed & don't take into account undo-restore logic (SCENE_FOREACH_UNDO_RESTORE), keeping for completeness.

As I understand it the root cause is as follows.

1) Internally Blender starts with the brush pointer set to "Add".
2) Entering vertex paint uses the the default brush to initialize set the brush pointer.
3) Entering vertex paint modifies the tool settings WITHOUT tagging the scene ID_RECALC_COPY_ON_WRITE.
4) Undo reads the value of the scene in it's initial internal state (where the Add brush is used).
5) Undo uses the ID's as the source of truth syncing the brush back to the tool-system.
6) Entering vertex paint mode then uses the wrong brush.

Point 3 is the cause of the problem.

So as far as I can see the fix for this is to always tag the scene ID_RECALC_COPY_ON_WRITE when it's tool settings are modified, otherwise undo might not include the changes.

There are many places that set the brush and they don't always have the owner ID available (the scene in this case). This PR !117055 is an ensures the scene is tagged which AFAICS is correct but could be re-thought or refactored to avoid similar accidents in the future.

*Edit* these findings look to be flawed & don't take into account undo-restore logic (`SCENE_FOREACH_UNDO_RESTORE`), keeping for completeness. ---- As I understand it the root cause is as follows. ``` 1) Internally Blender starts with the brush pointer set to "Add". 2) Entering vertex paint uses the the default brush to initialize set the brush pointer. 3) Entering vertex paint modifies the tool settings WITHOUT tagging the scene ID_RECALC_COPY_ON_WRITE. 4) Undo reads the value of the scene in it's initial internal state (where the Add brush is used). 5) Undo uses the ID's as the source of truth syncing the brush back to the tool-system. 6) Entering vertex paint mode then uses the wrong brush. ``` Point 3 is the cause of the problem. So as far as I can see the fix for this is to always tag the scene `ID_RECALC_COPY_ON_WRITE` when it's tool settings are modified, otherwise undo might not include the changes. There are many places that set the brush and they don't always have the owner ID available (the scene in this case). This PR !117055 is an ensures the scene is tagged which AFAICS is correct but could be re-thought or refactored to avoid similar accidents in the future.

Bastien Montagne commented

2024-01-12 11:04:23 +01:00

Owner

While tagging an ID when its data is modified is for sure required, I fail to understand why it solves this issue currently. To me this sounds more like another 'magic fix that happens to work'. I would expect the tool system to work with the original Scene data, not with some evaluated one from depsgraph...

As a reminder, I do not think that 4 is correct? The whole toolsettings are skipped on undo, it's always re-using the current (before undo step loading) data (see scene_undo_preserve). At least afair when investigating this last month, the brush was the expected 'Draw' one in scene's toolsettings after undo step reading. As said in my previous comment, it's the call to toolsystem_ref_link that ends up resetting that brush to the Add one. Could it be that this is actually working with evaluated Scene data???

Although this is definitely not a correct solution, I would be curious to know if replacing ID_RECALC_COPY_ON_WRITE by ID_RECALC_TAG_FOR_UNDO in your patch also does solve the issue? That later tag should have no effect on depsgraph/evaluation, and only enforces the ID to be detected as modified by the undo system.

While tagging an ID when its data is modified is for sure required, I fail to understand why it solves this issue currently. To me this sounds more like another 'magic fix that happens to work'. I would expect the tool system to work with the original Scene data, not with some evaluated one from depsgraph... As a reminder, I do not think that `4` is correct? The whole toolsettings are skipped on undo, it's always re-using the current (before undo step loading) data (see `scene_undo_preserve`). At least afair when investigating this last month, the brush was the expected 'Draw' one in scene's toolsettings after undo step reading. As said in my previous comment, it's the call to `toolsystem_ref_link` that ends up resetting that brush to the `Add` one. Could it be that this is actually working with evaluated Scene data??? Although this is definitely not a correct solution, I would be curious to know if replacing `ID_RECALC_COPY_ON_WRITE` by `ID_RECALC_TAG_FOR_UNDO` in your patch also does solve the issue? That later tag should have no effect on depsgraph/evaluation, and only enforces the ID to be detected as modified by the undo system.

Campbell Barton commented

2024-01-12 14:08:37 +01:00

Owner

It seems my initial findings were flawed because tagging ID_RECALC_COPY_ON_WRITE does NOT fix the bug, although when testing an earlier version of the patch I somehow managed to fix the issue, the final patch didn't.

After some more discussion, my understanding is that SCENE_FOREACH_UNDO_RESTORE should keep the paint-brush at it's current value (where possible) even if the undo step stores a different value (this part threw me).

Updated findings:

When reading the undo step that loads object mode scene_undo_preserve runs.
- scene_new->toolsettings->vpaint->paint.tool_slots[0].brush points to the "Add" brush.
- scene_old->toolsettings->vpaint->paint.tool_slots[0].brush points to the "Draw" brush.

... Reading scene_foreach_paint there is an attempt to keep the values of the old brushes. ...

BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER_P macro runs on the brush pointer, calling scene_foreach_toolsettings_id_pointer_process.
- the SCENE_FOREACH_UNDO_RESTORE case is used.
- The BLO_read_get_new_id_address_from_session_uuid lookup succeeds, but the pointer returned is the same as id_old, (id_old_new == id_old).
- This causes the if (!ELEM(id_old_new, id_old, nullptr)) {...} block to fail which is needed for the previous pointer to be restored.
- The std::swap(*id_p, *id_old_p); writes the "Add" brush into the "Draw" brushes ID, causing the brush to be "Add".

It seems BLO_read_get_new_id_address_from_session_uuid(..) should return a new pointer that doesn't match the existing pointer but I don't know how this would be done.

It seems my initial findings were flawed because tagging `ID_RECALC_COPY_ON_WRITE` does NOT fix the bug, although when testing an earlier version of the patch I somehow managed to fix the issue, the final patch didn't. After some more discussion, my understanding is that SCENE_FOREACH_UNDO_RESTORE should keep the paint-brush at it's current value (where possible) even if the undo step stores a different value (this part threw me). Updated findings: - When reading the undo step that loads object mode `scene_undo_preserve` runs. - `scene_new->toolsettings->vpaint->paint.tool_slots[0].brush` points to the "Add" brush. - `scene_old->toolsettings->vpaint->paint.tool_slots[0].brush` points to the "Draw" brush. ... Reading `scene_foreach_paint` there is an attempt to keep the values of the old brushes. ... - `BKE_LIB_FOREACHID_UNDO_PRESERVE_PROCESS_IDSUPER_P` macro runs on the brush pointer, calling `scene_foreach_toolsettings_id_pointer_process`. - the `SCENE_FOREACH_UNDO_RESTORE` case is used. - The `BLO_read_get_new_id_address_from_session_uuid` lookup succeeds, but the pointer returned is the same as `id_old`, `(id_old_new == id_old)`. - This causes the `if (!ELEM(id_old_new, id_old, nullptr)) {...}` block to fail which is needed for the previous pointer to be restored. - The `std::swap(*id_p, *id_old_p);` writes the "Add" brush into the "Draw" brushes ID, causing the brush to be "Add". ---- It seems `BLO_read_get_new_id_address_from_session_uuid(..)` should return a new pointer that doesn't match the existing pointer but I don't know how this would be done.

Bastien Montagne commented

2024-01-12 15:27:41 +01:00

Owner

Eeeeeh reading at code in scene_foreach_toolsettings_id_pointer_process, there is indeed one critical issue when both id_old and id_old_new pointers are the same! Then the code should essentially do nothing.

Eeeeeh reading at code in `scene_foreach_toolsettings_id_pointer_process`, there is indeed one critical issue when both `id_old` and `id_old_new` pointers are the same! Then the code should essentially do nothing.

Bastien Montagne referenced this issue from a commit

2024-01-12 15:55:01 +01:00

Fix #116029: Regression: Ctrl+Z breaks vertex paint.

Bastien Montagne closed this issue

2024-01-12 15:55:03 +01:00

Blender Bot added

and removed

labels 2024-01-12 15:55:05 +01:00

Sign in to join this conversation.

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

6 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#116029

No description provided.