GN mesh to curve to mesh causes hard crash with cage display #95479

New Issue

Nathan Vasil · 2022-02-03T20:14:38+01:00

Nathan Vasil commented

2022-02-03 20:14:38 +01:00

System Information
Operating system: Windows-10-10.0.19042-SP0 64 Bits
Graphics card: NVIDIA GeForce GTX 1070/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 511.65

Blender Version
Broken: version: 3.1.0 Beta, branch: master, commit date: 2022-02-02 15:25, hash: 43b0ff3054
Also broken: version: 3.0.0, branch: master, commit date: 2021-12-02 18:35, hash: f1cca30557
Also broken: version: 3.2.0 Alpha, branch: master, commit date: 2022-02-03 13:54, hash: 5920de9247

Short description of error

Cage display of geometry nodes mesh-to-curve -> curve to mesh causes hard crash with at least some geometry inputs.

Exact steps for others to reproduce the error
File has an object selected (with some verts selected)

Open .blend file
Tab into edit mode
Then numpad period to zoom on selection

Won't crash if we disable cage display. Won't crash if we only output the curve, without a profile definition. Will still crash if we use a different profile (like a curve circle.)

Numpad period is not necessary to for the crash; the cage display is generally unstable. Zooming is just a way to reliably crash right away in the file I provided.

Test File:
GNcurveAutotiltBug6.blend
GNcurveAutotiltBug6.crash.txt

**System Information** Operating system: Windows-10-10.0.19042-SP0 64 Bits Graphics card: NVIDIA GeForce GTX 1070/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 511.65 **Blender Version** Broken: version: 3.1.0 Beta, branch: master, commit date: 2022-02-02 15:25, hash: `43b0ff3054` Also broken: version: 3.0.0, branch: master, commit date: 2021-12-02 18:35, hash: `f1cca30557` Also broken: version: 3.2.0 Alpha, branch: master, commit date: 2022-02-03 13:54, hash: `5920de9247` **Short description of error** Cage display of geometry nodes mesh-to-curve -> curve to mesh causes hard crash with at least some geometry inputs. **Exact steps for others to reproduce the error** File has an object selected (with some verts selected) - Open .blend file - Tab into edit mode - Then numpad period to zoom on selection Won't crash if we disable cage display. Won't crash if we only output the curve, without a profile definition. Will still crash if we use a different profile (like a curve circle.) Numpad period is not necessary to for the crash; the cage display is generally unstable. Zooming is just a way to reliably crash right away in the file I provided. Test File: [GNcurveAutotiltBug6.blend](https://archive.blender.org/developer/F12844275/GNcurveAutotiltBug6.blend) [GNcurveAutotiltBug6.crash.txt](https://archive.blender.org/developer/F12844278/GNcurveAutotiltBug6.crash.txt)

Nathan Vasil commented

2022-02-03 20:14:38 +01:00

Added subscriber: @vasiln

Hans Goudey commented

2022-02-04 01:11:55 +01:00

Added subscriber: @HooglyBoogly

Hans Goudey commented

2022-02-04 01:11:55 +01:00

Hmm, I can't reproduce a crash zooming, selecting, or toggling cage display. I wonder if others can. I'm using latest master, b73d3b80fdcb724.

Hmm, I can't reproduce a crash zooming, selecting, or toggling cage display. I wonder if others can. I'm using latest master, `b73d3b80fdcb724`.

Nathan Vasil commented

2022-02-04 01:14:21 +01:00

It's not impossible that it's a video card thing, although I did download updated drivers just in case.

Missed more recent builds since the beta was latest on the downloads page; I'll check with the alpha I see there now.

Edit: Still broken in 3.2.0 Alpha, branch: master, commit date: 2022-02-03 13:54, hash: 5920de9247

It's not impossible that it's a video card thing, although I did download updated drivers just in case. Missed more recent builds since the beta was latest on the downloads page; I'll check with the alpha I see there now. Edit: Still broken in 3.2.0 Alpha, branch: master, commit date: 2022-02-03 13:54, hash: `5920de9247`

Pratik Borhade commented

2022-02-04 04:04:43 +01:00

Added subscriber: @PratikPB2123

Pratik Borhade commented

2022-02-04 04:04:43 +01:00

Changed status from 'Needs Triage' to: 'Confirmed'

Pratik Borhade commented

2022-02-04 04:04:43 +01:00

Thanks for the report. Can confirm on recent 3.1.0 build from buildbot but doesn't crash on current master(28656293c6)

Stack trace:
blender.exe         :0x00007FF6C0EC33A0  set_mapped_co
blender.exe         :0x00007FF6BBCDFE20  BKE_mesh_foreach_mapped_vert
blender.exe         :0x00007FF6C0EC2740  ED_transverts_create_from_obedit
blender.exe         :0x00007FF6BC7F7A20  ED_view3d_minmax_verts
blender.exe         :0x00007FF6BC7E8D10  viewselected_exec
blender.exe         :0x00007FF6BBD71040  wm_operator_invoke
blender.exe         :0x00007FF6BBD6EBE0  wm_handler_operator_call
blender.exe         :0x00007FF6BBD70410  wm_handlers_do_keymap_with_keymap_handler
blender.exe         :0x00007FF6BBD6FBD0  wm_handlers_do_intern
blender.exe         :0x00007FF6BBD6F200  wm_handlers_do
blender.exe         :0x00007FF6BBD6C280  wm_event_do_handlers
blender.exe         :0x00007FF6BBD563E0  WM_main
blender.exe         :0x00007FF6BB9A03E0  main
blender.exe         :0x00007FF6C1077744  __scrt_common_main_seh
KERNEL32.DLL        :0x00007FFA44AA7020  BaseThreadInitThunk
ntdll.dll           :0x00007FFA46942630  RtlUserThreadStart```

Thanks for the report. Can confirm on recent 3.1.0 build from buildbot but doesn't crash on current master(28656293c624612a72224e0c7139471188c1203e) ```lines=10 Stack trace: blender.exe :0x00007FF6C0EC33A0 set_mapped_co blender.exe :0x00007FF6BBCDFE20 BKE_mesh_foreach_mapped_vert blender.exe :0x00007FF6C0EC2740 ED_transverts_create_from_obedit blender.exe :0x00007FF6BC7F7A20 ED_view3d_minmax_verts blender.exe :0x00007FF6BC7E8D10 viewselected_exec blender.exe :0x00007FF6BBD71040 wm_operator_invoke blender.exe :0x00007FF6BBD6EBE0 wm_handler_operator_call blender.exe :0x00007FF6BBD70410 wm_handlers_do_keymap_with_keymap_handler blender.exe :0x00007FF6BBD6FBD0 wm_handlers_do_intern blender.exe :0x00007FF6BBD6F200 wm_handlers_do blender.exe :0x00007FF6BBD6C280 wm_event_do_handlers blender.exe :0x00007FF6BBD563E0 WM_main blender.exe :0x00007FF6BB9A03E0 main blender.exe :0x00007FF6C1077744 __scrt_common_main_seh KERNEL32.DLL :0x00007FFA44AA7020 BaseThreadInitThunk ntdll.dll :0x00007FFA46942630 RtlUserThreadStart```

Pratik Borhade commented

2022-02-04 04:33:34 +01:00

On master crash was fixed between c4bedeb018 - 0379ddac7d

Relevant commit I found which probably have caused the crash on 3.1 branch 974981a637 (Just a guess, I am not sure)

On master crash was fixed between c4bedeb0186b - 0379ddac7d68 Relevant commit I found which probably have caused the crash on 3.1 branch 974981a637 (Just a guess, I am not sure)

Hans Goudey commented

2022-02-04 07:11:02 +01:00

I can't reproduce this in 3.0.1. I can look into the 3.1 branch soon.

Hans Goudey commented

2022-02-04 21:13:54 +01:00

Added subscriber: @Sergey

Hans Goudey commented

2022-02-04 21:13:54 +01:00

Hi @Sergey, looking into this, I think it's related to your recent changes in this area, I can confirm that 974981a637 fixes this in master, but not in the realease branch. Do you mind taking a look? Thanks.

Hi @Sergey, looking into this, I think it's related to your recent changes in this area, I can confirm that 974981a637 fixes this in master, but not in the realease branch. Do you mind taking a look? Thanks.

Raimund Klink commented

2022-03-01 16:00:47 +01:00

Added subscriber: @Raimund58

Sergey Sharybin self-assigned this 2022-03-01 16:23:24 +01:00

Sergey Sharybin commented

2022-03-01 16:42:03 +01:00

The crash happens in 3.0 for me here.

There was a mistake done in 974981a637 which introduced some other issues and got fixed in the 71b451bb62. That brought 3.0 behavior back.

The crash happens in 3.0 for me here. There was a mistake done in 974981a637 which introduced some other issues and got fixed in the 71b451bb62. That brought 3.0 behavior back.

Sergey Sharybin removed their assignment 2022-03-01 17:23:51 +01:00

Sergey Sharybin commented

2022-03-01 17:23:51 +01:00

The root issue seems to be in the CD_ORIGINDEX layer. Or, rather, in absence of it. If a modifier does not preserve mapping it is supposed to set CD_ORIGINDEX elements to ORIGINDEX_NONE instead of deleting the layer all together.

From stepping through code in the modifier_modify_mesh_and_geometry_set the input_mesh has the CD_ORIGINDEX after BKE_mesh_wrapper_ensure_mdata. But the mesh_output = mesh_component.release(); does not.

Quick and dirty PoC to get proper behavior in this specific case: P2815: (An Untitled Masterwork)

diff --git a/source/blender/blenkernel/intern/DerivedMesh.cc b/source/blender/blenkernel/intern/DerivedMesh.cc
index 39074a5c75f..cf69e3ee6c1 100644
--- a/source/blender/blenkernel/intern/DerivedMesh.cc
+++ b/source/blender/blenkernel/intern/DerivedMesh.cc
@@ -710,6 +710,12 @@ static Mesh *modifier_modify_mesh_and_geometry_set(ModifierData *md,
         mesh_component.ensure_owns_direct_data();
       }
       mesh_output = mesh_component.release();
+      CustomData_add_layer(
+          &mesh_output->vdata, CD_ORIGINDEX, CD_DEFAULT, nullptr, mesh_output->totvert);
+      CustomData_add_layer(
+          &mesh_output->edata, CD_ORIGINDEX, CD_DEFAULT, nullptr, mesh_output->totedge);
+      CustomData_add_layer(
+          &mesh_output->pdata, CD_ORIGINDEX, CD_DEFAULT, nullptr, mesh_output->totpoly);
     }
 
     /* Return an empty mesh instead of null. */

It also makes it so that the cage is properly rendered in the edit mode: it is not expected in this file that in the object mode you see faces and don't see them in edit mode. Guess the drawing code checks for CD_ORIGINDEX somewhere, but this is not something we can do in the generic vertex looping function.

If the geoemtry nodes modifier does not support mapping it is not to advertise eModifierTypeFlag_SupportsMapping. If it supports mapping (including supports mapping in some cases) it must preserve existence of CD_ORIGINDEXand set "unknown" mapping to ORIGINDEX_NONE.

By now it doesn't seem to be a report in the area I am responsible for.

Addendum: The other possibility is to prevent geometry nodes from being applied on cage by removing eModifierTypeFlag_SupportsMapping flag from the modifier type info.

The root issue seems to be in the `CD_ORIGINDEX` layer. Or, rather, in absence of it. If a modifier does not preserve mapping it is supposed to set `CD_ORIGINDEX` elements to `ORIGINDEX_NONE` instead of deleting the layer all together. From stepping through code in the `modifier_modify_mesh_and_geometry_set` the `input_mesh` has the `CD_ORIGINDEX` after `BKE_mesh_wrapper_ensure_mdata`. But the `mesh_output = mesh_component.release();` does not. Quick and dirty PoC to get proper behavior in this specific case: [P2815: (An Untitled Masterwork)](https://archive.blender.org/developer/P2815.txt) ``` diff --git a/source/blender/blenkernel/intern/DerivedMesh.cc b/source/blender/blenkernel/intern/DerivedMesh.cc index 39074a5c75f..cf69e3ee6c1 100644 --- a/source/blender/blenkernel/intern/DerivedMesh.cc +++ b/source/blender/blenkernel/intern/DerivedMesh.cc @@ -710,6 +710,12 @@ static Mesh *modifier_modify_mesh_and_geometry_set(ModifierData *md, mesh_component.ensure_owns_direct_data(); } mesh_output = mesh_component.release(); + CustomData_add_layer( + &mesh_output->vdata, CD_ORIGINDEX, CD_DEFAULT, nullptr, mesh_output->totvert); + CustomData_add_layer( + &mesh_output->edata, CD_ORIGINDEX, CD_DEFAULT, nullptr, mesh_output->totedge); + CustomData_add_layer( + &mesh_output->pdata, CD_ORIGINDEX, CD_DEFAULT, nullptr, mesh_output->totpoly); } /* Return an empty mesh instead of null. */ ``` It also makes it so that the cage is properly rendered in the edit mode: it is not expected in this file that in the object mode you see faces and don't see them in edit mode. Guess the drawing code checks for `CD_ORIGINDEX` somewhere, but this is not something we can do in the generic vertex looping function. If the geoemtry nodes modifier does not support mapping it is **not** to advertise `eModifierTypeFlag_SupportsMapping`. If it supports mapping (including supports mapping in *some* cases) it **must** preserve existence of `CD_ORIGINDEX`and set "unknown" mapping to `ORIGINDEX_NONE`. By now it doesn't seem to be a report in the area I am responsible for. **Addendum**: The other possibility is to prevent geometry nodes from being applied on cage by removing `eModifierTypeFlag_SupportsMapping` flag from the modifier type info.

Jacques Lucke commented

2022-03-02 12:47:49 +01:00

Added subscriber: @JacquesLucke

Jacques Lucke commented

2022-03-02 12:47:49 +01:00

The issue is that sometimes, a node setup supports mapping, and sometimes it does not. Users expect it to work in the cases when it makes sense. Having a single static flag for all geometry nodes modifiers is just not flexible enough.
If other code expects the CD_ORIGINDEX layer in this case, I'm find with adding it back if it doesn't exist like in your patch. Longer term this flag system should be refactored to support the more dynamic nature of geometry nodes, and code further down the line should be able to handle the non-existance of custom data layers.
We had similar issues in the past where drawing code crashed because the set of attributes on the original and evaluated data did not match.

The issue is that sometimes, a node setup supports mapping, and sometimes it does not. Users expect it to work in the cases when it makes sense. Having a single static flag for all geometry nodes modifiers is just not flexible enough. If other code expects the `CD_ORIGINDEX` layer in this case, I'm find with adding it back if it doesn't exist like in your patch. Longer term this flag system should be refactored to support the more dynamic nature of geometry nodes, and code further down the line should be able to handle the non-existance of custom data layers. We had similar issues in the past where drawing code crashed because the set of attributes on the original and evaluated data did not match.

Sergey Sharybin commented

2022-03-02 13:22:54 +01:00

Having a single static flag for all geometry nodes modifiers is just not flexible enough.

I don't really understand why geometry nodes modifier can not treat CD_ORIGINDEX in the same way as modifiers like Boolean or Subsurf and set origindex to certain value when mapping is known, and set it to ORIGINDEX_NONE when mapping is not known.

While one might argue design of the modifier stack is limited, there are certain expectations about behavior of the modifier. We can not be hot-fixing design deviations of specific modifiers in the generic modifier stack evaluation function.

We can potentially save memory by removing CD_ORIGINDEX layers if the mapping is 1:1, but that needs to be done separately and carefully. Until then modifiers are to follow design they are expected to follow.

> Having a single static flag for all geometry nodes modifiers is just not flexible enough. I don't really understand why geometry nodes modifier can not treat `CD_ORIGINDEX` in the same way as modifiers like Boolean or Subsurf and set origindex to certain value when mapping is known, and set it to `ORIGINDEX_NONE` when mapping is not known. While one might argue design of the modifier stack is limited, there are certain expectations about behavior of the modifier. We can not be hot-fixing design deviations of specific modifiers in the generic modifier stack evaluation function. We can potentially save memory by removing `CD_ORIGINDEX` layers if the mapping is 1:1, but that needs to be done separately and carefully. Until then modifiers are to follow design they are expected to follow.

Jacques Lucke commented

2022-03-02 13:27:21 +01:00

Fair enough, adding the layers can also be moved into the modifier.
Part of the problem is that eModifierTypeFlag_SupportsMapping doesn't have a comment explaining what it actually expects from modifiers. I thought just not having this layer should indicate to everyone that the mapping didn't work.

Fair enough, adding the layers can also be moved into the modifier. Part of the problem is that `eModifierTypeFlag_SupportsMapping` doesn't have a comment explaining what it actually expects from modifiers. I thought just not having this layer should indicate to everyone that the mapping didn't work.

Sergey Sharybin commented

2022-03-02 13:36:17 +01:00

Part of the problem is that eModifierTypeFlag_SupportsMapping doesn't have a comment explaining what it actually expects from modifiers.

This is so much much a fair point.

I thought just not having this layer should indicate to everyone that the mapping didn't work.

A lot of the code considers lack of CD_ORIGINDEX mean 1:1 mapping. It's "ok" if the topology does not change. If it does then it leads to situations like this.

> Part of the problem is that `eModifierTypeFlag_SupportsMapping` doesn't have a comment explaining what it actually expects from modifiers. This is so much much a fair point. > I thought just not having this layer should indicate to everyone that the mapping didn't work. A lot of the code considers lack of `CD_ORIGINDEX` mean 1:1 mapping. It's "ok" if the topology does not change. If it does then it leads to situations like this.

Hans Goudey commented

2022-03-02 13:55:11 +01:00

In #95479#1316097, @Sergey wrote:
A lot of the code considers lack of CD_ORIGINDEX mean 1:1 mapping. It's "ok" if the topology does not change. If it does then it leads to situations like this.

Aha, I think I finally understand the problem with not having the layer. Thank you.

I'll second Jacques' point and say that it's really hard to piece together this design when it doesn't seem to be described anywhere. A comment somewhere would be very helpful.

> In #95479#1316097, @Sergey wrote: > A lot of the code considers lack of `CD_ORIGINDEX` mean 1:1 mapping. It's "ok" if the topology does not change. If it does then it leads to situations like this. Aha, I think I finally understand the problem with not having the layer. Thank you. I'll second Jacques' point and say that it's really hard to piece together this design when it doesn't seem to be described anywhere. A comment somewhere would be very helpful.

blender-admin commented

2022-03-02 16:10:42 +01:00

This issue was referenced by 7935d3668a

This issue was referenced by 7935d3668a937ec5046558f9ff885a42a87b3384

Jacques Lucke commented

2022-03-02 16:11:03 +01:00

Changed status from 'Confirmed' to: 'Resolved'

Jacques Lucke closed this issue

2022-03-02 16:11:03 +01:00

Jacques Lucke self-assigned this 2022-03-02 16:11:03 +01:00

Thomas Dinges added this to the 3.1 milestone 2023-02-08 15:52:54 +01:00

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

GN mesh to curve to mesh causes hard crash with cage display #95479