Assert in debug for dyntopo sculpt #111574

New Issue

Iliya Katushenock · 2023-08-27T02:39:32+02:00

Iliya Katushenock commented

2023-08-27 02:39:32 +02:00

System Information
Operating system: Windows-10-10.0.19045-SP0 64 Bits
Graphics card: NVIDIA GeForce GTX 1060 6GB/PCIe/SSE2 NVIDIA Corporation 4.6.0 NVIDIA 527.56

Blender Version
Broken: Today main.
Worked: (newest version of Blender that worked as expected)

Short description of error
See attached stack trace file.

Exact steps for others to reproduce the error
In debug:

Sculp mode.
Enable Dyntopo option.
Do something.
See the crash.

**System Information** Operating system: Windows-10-10.0.19045-SP0 64 Bits Graphics card: NVIDIA GeForce GTX 1060 6GB/PCIe/SSE2 NVIDIA Corporation 4.6.0 NVIDIA 527.56 **Blender Version** Broken: Today main. Worked: (newest version of Blender that worked as expected) **Short description of error** See attached stack trace file. **Exact steps for others to reproduce the error** In debug: 1. Sculp mode. 2. Enable Dyntopo option. 3. Do something. See the crash.

dyntopo assert.txt

33 KiB

Iliya Katushenock added the

labels 2023-08-27 02:39:33 +02:00

Iliya Katushenock commented

2023-08-27 02:41:19 +02:00

Can you take a look at this @HooglyBoogly /

Hans Goudey self-assigned this 2023-08-27 15:15:12 +02:00

Pratik Borhade commented

2023-08-28 05:50:37 +02:00

I can confirm the assert hit.

Pratik Borhade added

Module

Sculpt, Paint & Texture

and removed

labels 2023-08-28 05:51:50 +02:00

Evan Wilson changed title from ~~Regression: Asser in debug for dyntopo sculpt~~ to Regression: Assert in debug for dyntopo sculpt

2023-08-28 06:35:50 +02:00

Hans Goudey commented

2023-08-29 03:17:23 +02:00

I tested main from 1 month ago and the assert still triggered. I don't understand what the assert is supposed to test for, so not sure there's much I can do here.

👍 1

Hans Goudey removed their assignment 2023-08-29 03:17:29 +02:00

Pratik Borhade commented

2023-08-29 05:47:15 +02:00

I'll lower the priority then.
(Also unclear to us whether this is a regression or not)

I'll lower the priority then. (Also unclear to us whether this is a regression or not)

Pratik Borhade added

and removed

labels 2023-08-29 05:47:27 +02:00

Hans Goudey changed title from ~~Regression: Assert in debug for dyntopo sculpt~~ to Assert in debug for dyntopo sculpt

2023-08-29 05:47:57 +02:00

Pratik Borhade referenced this issue

2023-10-05 05:40:01 +02:00

Dyntopo on the default cube asserts in 4.0 #113271

Pratik Borhade referenced this issue

2024-02-16 06:34:26 +01:00

Assert failure in BMesh when Dyntopo is enabled #118356

Sean Kim commented

2024-02-17 00:47:22 +01:00

Started looking into this because I'm doing a bit of work on dyntopo related features.

The commit that added a lot of the relevant code is here: 7f133b7a38

I'll do a build of this older commit to see if this assert has failed since this was added, but I'm confused as to how this codepath doesn't cause a segfault with dereferencing a nullptr when the assert is disabled.

  BLI_assert(entry);
  BLI_assert(BLI_ghash_haskey(entry->modified_verts, key));
  lv = static_cast<const BMLogVert *>(BLI_ghash_lookup(entry->modified_verts, key));
  return lv->co;

If BLI_ghash_haskey is returning false, then BLI_ghash_lookup should return NULL which I assume would cause lv->co to then crash blender.

Started looking into this because I'm doing a bit of work on dyntopo related features. The commit that added a lot of the relevant code is here: https://projects.blender.org/blender/blender/commit/7f133b7a38341a245ff4cfab0e18a271c021bd94 I'll do a build of this older commit to see if this assert has failed since this was added, but I'm confused as to how this codepath doesn't cause a segfault with dereferencing a nullptr when the assert is disabled. ``` BLI_assert(entry); BLI_assert(BLI_ghash_haskey(entry->modified_verts, key)); lv = static_cast<const BMLogVert *>(BLI_ghash_lookup(entry->modified_verts, key)); return lv->co; ``` If `BLI_ghash_haskey` is returning false, then `BLI_ghash_lookup` should return `NULL` which I assume would cause `lv->co` to then crash blender.

Hans Goudey commented

2024-02-17 01:22:35 +01:00

Yeah, this assert has been here since I started dealing with dyntopo code last year or the year before.

It doesn't segfault because offsetof(BMVert::co) == 0, and the caller has a null check on the returned float pointer.

But I still can't make any sense of this area, so I don't really have a suggestion besides "remove the assert, add some more null checks"

Yeah, this assert has been here since I started dealing with dyntopo code last year or the year before. It doesn't segfault because `offsetof(BMVert::co) == 0`, and the caller has a null check on the returned float pointer. But I still can't make any sense of this area, so I don't really have a suggestion besides "remove the assert, add some more null checks"

Sean Kim commented

2024-02-17 02:08:42 +01:00

Building the code has it crash to an entirely different assert well before it even gets to the topology update. I don't think further digging is going to be particularly useful.

The original assert according to the git history has been around since this file was made back in 2012 - my current understanding of the code added that's calling BM_log_original_vert_co is that it was used for some transitionary code for a deprecated attribute of the PBVH.

It seems like an oversight of the calling code in the end - BM_log_original_vert_co doesn't have a "safe" variant and just assumes that if you're requesting a vertex it has to be in the BMesh Log.

I can see a few options:

Adjust the conditional before calling BM_log_original_vert_co so that it checks for use_original before even attempting to access the log, since the sculpt codepath that is triggering this assert doesnt even want this information.
Add a BM_log_has_vert method and do something similar to 1 so the solution is slightly more robust.
Dig really far into the bm_orco usage and see if it can be safely removed and this update step entirely removed.

Building the code has it crash to an entirely different assert well before it even gets to the topology update. I don't think further digging is going to be particularly useful. The original assert according to the git history has been around since this file was made back in 2012 - my current understanding of the code added that's calling `BM_log_original_vert_co` is that it was used for some transitionary code for a deprecated attribute of the PBVH. It seems like an oversight of the calling code in the end - `BM_log_original_vert_co` doesn't have a "safe" variant and just assumes that if you're requesting a vertex it has to be in the BMesh Log. I can see a few options: 1. Adjust the conditional before calling `BM_log_original_vert_co` so that it checks for `use_original` before even attempting to access the log, since the sculpt codepath that is triggering this assert doesnt even want this information. 2. Add a `BM_log_has_vert` method and do something similar to 1 so the solution is slightly more robust. 3. Dig really far into the `bm_orco` usage and see if it can be safely removed and this update step entirely removed.

Sean Kim referenced a pull request that will close this issue

2024-02-21 03:07:41 +01:00

Fix #111574: Avoid asserts in BMLog with Dyntopo #118543

Hans Goudey referenced this issue from a commit

2024-02-27 17:18:58 +01:00

Fix #111574: Avoid asserts in BMLog with Dyntopo

Hans Goudey closed this issue

2024-02-27 17:18:59 +01:00

Blender Bot added

Status

Resolved

and removed