Fix #122947: Curve stroke on duplicate object doesn't update normals #124268

Sean Kim · 2024-07-06T04:17:19+02:00

Sean Kim commented

2024-07-06 04:17:19 +02:00

When a mesh is shared between multiple objects, sculpting with a brush
with the Curve stroke type doesnt update normal values for the affected
nodes when using PBVH drawing. This is because when reevaluating the
depsgraph for the objects, the shared PBVH is destroyed and the nodes
are recalculated, losing the existing node flag updates.

This only occurs for the Curve stroke type because all of its stroke
steps are performed within a single call to the overall operator when
the user presses enter, unlike other brush strokes which apply on each
mouse movement.

To fix this, we simply force update the normals before destroying the
PBVH at the end of the stroke step.

When a mesh is shared between multiple objects, sculpting with a brush with the Curve stroke type doesnt update normal values for the affected nodes when using PBVH drawing. This is because when reevaluating the depsgraph for the objects, the shared PBVH is destroyed and the nodes are recalculated, losing the existing node flag updates. This only occurs for the Curve stroke type because all of its stroke steps are performed within a single call to the overall operator when the user presses enter, unlike other brush strokes which apply on each mouse movement. To fix this, we simply force update the normals before destroying the PBVH at the end of the stroke step.

Sean Kim added 1 commit 2024-07-06 04:17:28 +02:00

Fix #112947 : Curve stroke on duplicate object doesn't update normals b4e0d5dabf

When a mesh is shared between multiple objects, sculpting with a brush
with the Curve stroke type doesnt update normal values for the affected
nodes when using PBVH drawing. This is because when reevaluating the
depsgraph for the objects, the shared PBVH is destroyed and the nodes
are recalculated, losing the existing node flag updates.

This only occurs for the Curve stroke type because all of its stroke
steps are performed within a single call to the overall operator when
the user presses enter, unlike other brush strokes which apply on each
mouse movement.

Sean Kim added this to the Sculpt, Paint & Texture project 2024-07-06 04:17:47 +02:00

Sean Kim requested review from Sergey Sharybin 2024-07-06 04:17:56 +02:00

Sean Kim requested review from Hans Goudey 2024-07-06 04:18:02 +02:00

Iliya Katushenock commented

2024-07-06 10:02:26 +02:00

It seems #report ID is wrong..\

Sean Kim changed title from ~~Fix #112947: Curve stroke on duplicate object doesn't update normals~~ to Fix #122947: Curve stroke on duplicate object doesn't update normals

2024-07-06 10:05:49 +02:00

Sean Kim commented

2024-07-06 10:06:11 +02:00

It seems #report ID is wrong..\

Whoops, you're right. Corrected it now.

> It seems #report ID is wrong..\ Whoops, you're right. Corrected it now.

Sergey Sharybin reviewed 2024-07-08 12:13:36 +02:00

Sergey Sharybin left a comment

This might be the least evil if we want the issue to be fixed in 4.2, but it is still very evil thing to do.Disabling PBVH drawing has performance impact, and doing it based on the user count is not exactly correct: mesh might be referenced by a driver, or an object from an inactive scene.

What I don't really get, is why other brushes works fine, and this does not? Can we make some "missing" update to happen in this operator which handles multiple steps, to mimmic what happens with other brushes more closely?

This might be the least evil if we want the issue to be fixed in 4.2, but it is still very evil thing to do.Disabling PBVH drawing has performance impact, and doing it based on the user count is not exactly correct: mesh might be referenced by a driver, or an object from an inactive scene. What I don't really get, is why other brushes works fine, and this does not? Can we make some "missing" update to happen in this operator which handles multiple steps, to mimmic what happens with other brushes more closely?

Hans Goudey commented

2024-07-08 15:31:00 +02:00

What I don't really get, is why other brushes works fine, and this does not?

Yeah, this is the confusing part. The difference is that for the curve stroke type, all the samples are calculated in one execution of the modal operator. Then the PBVH is destroyed and the normal update tags are lost. For non-curve stroke types, each sample happens in its own execution of the modal callback. So it's only the last update tags that are lost.

Long term I think it would be great if we stopped clearing the PBVH for every single reevaluation. We should only do that when the mesh topology actually changes. And we actually track that nowadays, whereas we didn't years ago when this was written. Obviously we wouldn't do that for 4.2, but doing it soonish would be great, especially for things like node tools.

>What I don't really get, is why other brushes works fine, and this does not? Yeah, this is the confusing part. The difference is that for the curve stroke type, all the samples are calculated in one execution of the modal operator. Then the PBVH is destroyed and the normal update tags are lost. For non-curve stroke types, each sample happens in its own execution of the modal callback. So it's only the _last_ update tags that are lost. Long term I think it would be great if we stopped clearing the PBVH for every single reevaluation. We should only do that when the mesh topology actually changes. And we actually track that nowadays, whereas we didn't years ago when this was written. Obviously we wouldn't do that for 4.2, but doing it soonish would be great, especially for things like node tools.

Sergey Sharybin commented

2024-07-08 15:50:49 +02:00

Can we handle pending PBVH node updates before we destroy it?

Hans Goudey commented

2024-07-08 15:59:24 +02:00

That might be better. Downside is that we may be recalculating normals unnecessarily before object evaluation where positions may be deformed anyway. But it does feel more sound.

Sean Kim commented

2024-07-08 16:56:11 +02:00

Handling the updates before flushing the mesh seems like a potentially risky change. It's processed inside draw_sculpt.cc here so I suppose the suggestion would be to clear it before returning from this method, or elsewhere in the draw code?

Handling the updates before flushing the mesh seems like a potentially risky change. It's processed inside `draw_sculpt.cc` [here](https://projects.blender.org/blender/blender/src/branch/main/source/blender/draw/intern/draw_sculpt.cc#L100) so I suppose the suggestion would be to clear it before returning from this method, or elsewhere in the draw code?

Hans Goudey commented

2024-07-08 16:58:04 +02:00

Adding pbvh::update_normals right before the call to sculptsession_free_pbvh would probably do the trick. It would be worth testing that for multires though, in case the evaluated SubdivCCG is somehow cleared before that.

Adding `pbvh::update_normals` right before the call to `sculptsession_free_pbvh` would probably do the trick. It would be worth testing that for multires though, in case the evaluated `SubdivCCG` is somehow cleared before that.

Sean Kim added 2 commits 2024-07-08 18:20:43 +02:00

Revert "Fix #112947 : Curve stroke on duplicate object doesn't update normals" 97cb583865

This reverts commit b4e0d5dabf.

Update normals before destroying PBVH in depsgraph eval ca00e297f5

Hans Goudey reviewed 2024-07-08 18:57:05 +02:00

source/blender/blenkernel/intern/paint.cc Outdated

						
				@ -1856,2 +1856,4 @@

				  if (ss && ss->building_vp_handle == false) {

				    if (!ss->cache && !ss->filter_cache && !ss->expand_cache) {

				      if (ss->pbvh) {

				        /* Before destroying the pbvh, flush the updates to the underlying representation to ensure

Hans Goudey commented

2024-07-08 18:56:47 +02:00

I'll just pick on this comment a little-- I think it's a little more specific than it needs to be, it could mentioned the code of the problem a little more precisely too. I'd suggest:

PBVH nodes may contain dirty normal tags. To avoid losing that information when the PBVH is deleted, make sure all tagged geometry normals are up to date.

I'll just pick on this comment a little-- I think it's a little more specific than it needs to be, it could mentioned the code of the problem a little more precisely too. I'd suggest: >PBVH nodes may contain dirty normal tags. To avoid losing that information when the PBVH is deleted, make sure all tagged geometry normals are up to date.

👍 1

Sean-Kim marked this conversation as resolved

Sean Kim added 1 commit 2024-07-08 19:02:37 +02:00

Update comment 92f8544592

Hans Goudey approved these changes 2024-07-08 19:07:58 +02:00

Sean Kim merged commit d527e3a6bd into blender-v4.2-release

2024-07-08 21:36:41 +02:00

Sean Kim referenced this issue from a commit

2024-07-08 21:36:43 +02:00

Fix #122947: Curve stroke on duplicate object doesn't update normals

Sean Kim deleted branch fix-112947

2024-07-08 21:36:49 +02:00

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #122947: Curve stroke on duplicate object doesn't update normals #124268