Gpencil: Geometry nodes instance on points with GP crash on last keyframe delete #125866

New Issue

Aleš Jelovčan · 2024-08-04T16:28:21+02:00

Aleš Jelovčan commented

2024-08-04 16:28:21 +02:00

System Information
Operating system: Windows-10-10.0.22631-SP0 64 Bits
Graphics card: NVIDIA GeForce RTX 4090 Laptop GPU/PCIe/SSE2 NVIDIA Corporation 4.6.0 NVIDIA 537.32

Blender Version
Broken: version: 4.3.0 Alpha, branch: main, commit date: 2024-08-03 09:19, hash: e6178be3b855
Worked: (newest version of Blender that worked as expected)

Short description of error
If GPv3 is used as a Points input in instance on points, Blender will crash when last frame is deleted

Exact steps for others to reproduce the error
Delete keyframe in attached file

**System Information** Operating system: Windows-10-10.0.22631-SP0 64 Bits Graphics card: NVIDIA GeForce RTX 4090 Laptop GPU/PCIe/SSE2 NVIDIA Corporation 4.6.0 NVIDIA 537.32 **Blender Version** Broken: version: 4.3.0 Alpha, branch: main, commit date: 2024-08-03 09:19, hash: `e6178be3b855` Worked: (newest version of Blender that worked as expected) **Short description of error** If GPv3 is used as a Points input in instance on points, Blender will crash when last frame is deleted <img width="569" alt="image" src="attachments/fd5b5671-1caf-4992-b424-a0e1466cbb07"> **Exact steps for others to reproduce the error** Delete keyframe in attached file

image.png

110 KiB

gp-geo-delete-crash.blend

669 KiB

Aleš Jelovčan added the

labels 2024-08-04 16:28:22 +02:00

YimingWu commented

2024-08-05 06:49:02 +02:00

Looks pretty complex for me...

What I can see is that the custom data layer will have a null anonymous_id_ which I have no idea what's causing it...

Looks pretty complex for me... What I can see is that the custom data layer will have a null `anonymous_id_` which I have no idea what's causing it...

YimingWu added

and removed

labels 2024-08-05 06:51:13 +02:00

Philipp Oeser commented

2024-08-05 09:20:36 +02:00

Prefer if crashes are always High priority

👍 1

Philipp Oeser added

and removed

labels 2024-08-05 09:20:46 +02:00

Lukas Tönne commented

2024-08-05 10:25:08 +02:00

It seems the bke::Instances data is actually null after deleting the keyframe (the component exists but the internal pointer is null). The same happens if i just move the keyframe instead of deleting it. This data should be created in advance, not sure what is going on.

It seems the `bke::Instances` data is actually null after deleting the keyframe (the component exists but the internal pointer is null). The same happens if i just _move_ the keyframe instead of deleting it. This data should be created in advance, not sure what is going on.

Lukas Tönne commented

2024-08-05 11:28:37 +02:00

The issue is that the code assumes a valid InstancesComponent pointer when it transfers attributes from layers to instances.

		source/blender/nodes/geometry/nodes/node_geo_instance_on_points.cc
		Lines 257 to 260 in e981389bdd
	
				      GeometrySet::propagate_attributes_from_layer_to_instances(

				          geometry_set.get_grease_pencil()->attributes(),

				          geometry_set.get_instances_for_write()->attributes_for_write(),

				          propagation_info);

But geometry_set.get_instances_for_write() can return a nullptr even if the component exists (which is ensured by the code above).

		source/blender/nodes/geometry/nodes/node_geo_instance_on_points.cc
		Lines 193 to 198 in e981389bdd
	
				    InstancesComponent &instances_component =

				        geometry_set.get_component_for_write<InstancesComponent>();

				    bke::Instances *dst_instances = instances_component.get_for_write();

				    if (dst_instances == nullptr) {

				      dst_instances = new bke::Instances();

				      instances_component.replace(dst_instances);

This is because the GeometrySet::get_component_ptr function checks GeometrySet::has(type) which in turn checks both component.has_value() and component->is_empty():

		source/blender/blenkernel/intern/geometry_set.cc
		Line 152 in e981389bdd
	
				  return component.has_value() && !component->is_empty();

So the fix is to simply check if the instance component "exists" before accessing it:

-      GeometrySet::propagate_attributes_from_layer_to_instances(
-          geometry_set.get_grease_pencil()->attributes(),
-          geometry_set.get_instances_for_write()->attributes_for_write(),
-          propagation_info);
+      if (geometry_set.has_instances()) {
+        GeometrySet::propagate_attributes_from_layer_to_instances(
+            geometry_set.get_grease_pencil()->attributes(),
+            geometry_set.get_instances_for_write()->attributes_for_write(),
+            propagation_info);
+      }

The is_empty check here does not make sense to me (but probably has to stay because it's everywhere now). Getting a pointer to the instances geometry should not be contingent on the component having an actual instance (IMO). I guess we just have to live with that.

The issue is that the code assumes a valid `InstancesComponent` pointer when it transfers attributes from layers to instances. https://projects.blender.org/blender/blender/src/commit/e981389bddb1e5b79b30c296edba63ceb22a6942/source/blender/nodes/geometry/nodes/node_geo_instance_on_points.cc#L257-L260 But `geometry_set.get_instances_for_write()` can return a nullptr __even if the component exists__ (which is ensured by the code above). https://projects.blender.org/blender/blender/src/commit/e981389bddb1e5b79b30c296edba63ceb22a6942/source/blender/nodes/geometry/nodes/node_geo_instance_on_points.cc#L193-L198 This is because the `GeometrySet::get_component_ptr` function checks `GeometrySet::has(type)` which in turn checks both `component.has_value()` and `component->is_empty()`: https://projects.blender.org/blender/blender/src/commit/e981389bddb1e5b79b30c296edba63ceb22a6942/source/blender/blenkernel/intern/geometry_set.cc#L152 So the fix is to simply check if the instance component "exists" before accessing it: ```diff - GeometrySet::propagate_attributes_from_layer_to_instances( - geometry_set.get_grease_pencil()->attributes(), - geometry_set.get_instances_for_write()->attributes_for_write(), - propagation_info); + if (geometry_set.has_instances()) { + GeometrySet::propagate_attributes_from_layer_to_instances( + geometry_set.get_grease_pencil()->attributes(), + geometry_set.get_instances_for_write()->attributes_for_write(), + propagation_info); + } ``` The `is_empty` check here does not make sense to me (but probably has to stay because it's everywhere now). Getting a pointer to the instances geometry should not be contingent on the component having an actual instance (IMO). I guess we just have to live with that.

👀 1

Lukas Tönne referenced a pull request that will close this issue

2024-08-05 11:36:40 +02:00

Fix #125866: Crash when converting empty grease pencil to instances #125887

Lukas Tönne referenced this issue from a commit

2024-08-05 12:15:17 +02:00

Fix #125866: Crash when converting empty grease pencil to instances

Lukas Tönne closed this issue

2024-08-05 12:15:18 +02:00

Blender Bot added

Status

Resolved