Crash when dragging modifiers in Outliner #82945

New Issue

Erik Abrahamsson · 2020-11-23T17:05:04+01:00

Erik Abrahamsson commented

2020-11-23 17:05:04 +01:00

System Information
Operating system: Windows-10-10.0.18362-SP0 64 Bits
Graphics card: GeForce RTX 2080/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 441.66

Blender Version
Broken: version: 2.92.0 Alpha, branch: master, commit date: 2020-11-22 18:21, hash: 7bab87c119
Worked: (newest version of Blender that worked as expected)

Short description of error
Crash when dragging modifiers in Outliner.

Exact steps for others to reproduce the error
From the default startup file:

Duplicate the cube and move it a bit to the side.
Add a Particle system to the original cube. Also enable Dynamic Paint, choose type: Brush and Add Brush with the created Particle System as source.
In the Outliner, first drag the Particle System to the Cube.001, then the Dynamic Paint.
Press Ctrl+Z to crash Blender.

First two steps already done in #82945.blend

**System Information** Operating system: Windows-10-10.0.18362-SP0 64 Bits Graphics card: GeForce RTX 2080/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 441.66 **Blender Version** Broken: version: 2.92.0 Alpha, branch: master, commit date: 2020-11-22 18:21, hash: `7bab87c119` Worked: (newest version of Blender that worked as expected) **Short description of error** Crash when dragging modifiers in Outliner. **Exact steps for others to reproduce the error** From the default startup file: 1. Duplicate the cube and move it a bit to the side. 2. Add a Particle system to the original cube. Also enable Dynamic Paint, choose type: Brush and Add Brush with the created Particle System as source. 3. In the Outliner, first drag the Particle System to the Cube.001, then the Dynamic Paint. 4. Press Ctrl+Z to crash Blender. First two steps already done in [#82945.blend](https://archive.blender.org/developer/F9585215/T82945.blend)

Erik Abrahamsson commented

2020-11-23 17:05:04 +01:00

Added subscriber: @erik85

Philipp Oeser commented

2020-11-23 17:26:23 +01:00

Added subscriber: @lichtwerk

Philipp Oeser commented

2020-11-23 17:26:23 +01:00

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

Philipp Oeser commented

2020-11-23 17:26:23 +01:00

Can confirm, same thing does not happen when doing similar with make links > modifiers, then Undo.

ParticleSystemModifierData lacks a psys here:

1   psys_emitter_customdata_mask                                                                                                                                                                                                                                                particle.c           2246 0x3403d81 
2   requiredDataMask                                                                                                                                                                                                                                                            MOD_particlesystem.c 105  0x3e887ea 
3   BKE_modifier_calc_data_masks                                                                                                                                                                                                                                                modifier.c           581  0x33b160a 
4   mesh_calc_modifiers                                                                                                                                                                                                                                                         DerivedMesh.c        948  0x377b56a 
5   mesh_build_data                                                                                                                                                                                                                                                             DerivedMesh.c        1788 0x377df53 
6   makeDerivedMesh                                                                                                                                                                                                                                                             DerivedMesh.c        1925 0x377e463 
7   BKE_object_handle_data_update                                                                                                                                                                                                                                               object_update.c      192  0x33f304e 
8   BKE_object_eval_uber_data                                                                                                                                                                                                                                                   object_update.c      384  0x33f39b5

Can confirm, same thing does not happen when doing similar with make links > modifiers, then Undo. ParticleSystemModifierData lacks a psys here: ``` 1 psys_emitter_customdata_mask particle.c 2246 0x3403d81 2 requiredDataMask MOD_particlesystem.c 105 0x3e887ea 3 BKE_modifier_calc_data_masks modifier.c 581 0x33b160a 4 mesh_calc_modifiers DerivedMesh.c 948 0x377b56a 5 mesh_build_data DerivedMesh.c 1788 0x377df53 6 makeDerivedMesh DerivedMesh.c 1925 0x377e463 7 BKE_object_handle_data_update object_update.c 192 0x33f304e 8 BKE_object_eval_uber_data object_update.c 384 0x33f39b5 ``` ```

Fabrício Luis commented

2020-11-24 02:48:15 +01:00

Added subscriber: @ce3po

Bastien Montagne commented

2021-01-06 17:25:46 +01:00

Added subscriber: @mont29

Bastien Montagne self-assigned this 2021-01-06 17:25:46 +01:00

Bastien Montagne commented

2021-01-06 17:25:46 +01:00

Need to double check that, NULL psys here seems suspicious, but might actually be expected...

Bastien Montagne commented

2021-01-18 12:53:26 +01:00

Added subscriber: @brecht

Bastien Montagne commented

2021-01-18 12:53:26 +01:00

sigh This is actually a critical design flow in both ParticleSystem and DynamicPaint modifiers/systems, who store a pointer to some non-ID data that they do not own (the particle system, psys).

Then when copying those modifiers' data towards a new object, the psys pointers keep pointing at the one from the original object, not the new one owning the new modifiers.

This simply cannot work, think reference to the psys here should be stored as name (as we do e.g. for vgroups). This is the only way to be decoupled from a specific object (tbh, am not even sure how this did not backfire at us much more, I would expect this issue to also be a problem in CoW/despgraph context e.g.).

Note that DynamicPaint modifier seems to also have same issues with its canvas and brush pointers, but think here we could rather treat those pointers as runtime data (and hence reset them to NULL on copy), since there can only be one brush/canvas per object currently?

@brecht summoning you here to get a second opinion.

Raising to high as this is a fairly critical design flow, even though not technically a regression...

*sigh* This is actually a critical design flow in both ParticleSystem and DynamicPaint modifiers/systems, who store a pointer to some non-ID data that they do not own (the particle system, `psys`). Then when copying those modifiers' data towards a new object, the `psys` pointers keep pointing at the one from the original object, not the new one owning the new modifiers. This simply cannot work, think reference to the psys here should be stored as name (as we do e.g. for vgroups). This is the only way to be decoupled from a specific object (tbh, am not even sure how this did not backfire at us much more, I would expect this issue to also be a problem in CoW/despgraph context e.g.). Note that DynamicPaint modifier seems to also have same issues with its `canvas` and `brush` pointers, but think here we could rather treat those pointers as runtime data (and hence reset them to NULL on copy), since there can only be one brush/canvas per object currently? @brecht summoning you here to get a second opinion. Raising to high as this is a fairly critical design flow, even though not technically a regression...

Brecht Van Lommel commented

2021-01-18 13:03:31 +01:00

I think both the modifier and particle system are part of the same object datablock, so I'm not sure this is strictly against the Blender design?

Copying the particle system modifier should also copy the particle system, and remapping the pointer would be done along with that. For dynamic paint it's not obvious what it should do, if there is no particle system with the same name on the target object I guess any particle system pointer should be cleared.

Or copying these modifier could just not be supported for now.

I think both the modifier and particle system are part of the same object datablock, so I'm not sure this is strictly against the Blender design? Copying the particle system modifier should also copy the particle system, and remapping the pointer would be done along with that. For dynamic paint it's not obvious what it should do, if there is no particle system with the same name on the target object I guess any particle system pointer should be cleared. Or copying these modifier could just not be supported for now.

Erik Abrahamsson commented

2021-01-18 13:11:13 +01:00

If it helps, the new operator I made to copy modifiers to selected objects (D9537) also copies the particle system to the new object if a particle system with the same settings doesn't exist already.

If it helps, the new operator I made to copy modifiers to selected objects ([D9537](https://archive.blender.org/developer/D9537)) also copies the particle system to the new object if a particle system with the same settings doesn't exist already.

Bastien Montagne commented

2021-01-18 13:56:44 +01:00

This should not be handled at operator level, as copying of modifiers happens every time you copy and Object ID (including CoW copying e.g.).

@brecht the problem is that modifier copy code has no way to tell for which object it is copying data, and it should most definitively never add dependency to new IDs (like what would happen if creating a new particle system using a new particle settings).

That is why I don't think it should hold pointers to data that do not belong to itself, because it should not and cannot manage that data properly. So a lose reference (by names) should be used instead, just like what we do with e.g. vertex groups.
[EDIT] And then higher-level code (operators) can actually duplicate or add other data as needed.

Not supporting duplication of those modifiers is not an option, again because it would break depsgraph/CoW handling.

This should not be handled at operator level, as copying of modifiers happens every time you copy and Object ID (including CoW copying e.g.). @brecht the problem is that modifier copy code has no way to tell for which object it is copying data, and it should most definitively *never* add dependency to new IDs (like what would happen if creating a new particle system using a new particle settings). That is why I don't think it should hold pointers to data that do not belong to itself, because it should not and cannot manage that data properly. So a lose reference (by names) should be used instead, just like what we do with e.g. vertex groups. [EDIT] And then higher-level code (operators) can actually duplicate or add other data as needed. Not supporting duplication of those modifiers is not an option, again because it would break depsgraph/CoW handling.

Brecht Van Lommel commented

2021-01-18 14:34:24 +01:00

I agree that the design is not great here, and I hope that with a new particle system we simply store all settings in the modifier / node rather than in a separate list somewhere.

However I don't think that a loose coupling based on name really improves the situation. You'd still need all the same code to make sure the particle system is copied along with the modifier, and new code will be needed to keep the link valid when the particle system is renamed. It makes things safer against crashes, but does not really improve the architecture in my opinion.

I suggest putting the code from D9537: Copy a single modifier to all selected objects. into a function that can be reused for the outliner, and ensure we get the design right when a new particle system is added.

I agree that the design is not great here, and I hope that with a new particle system we simply store all settings in the modifier / node rather than in a separate list somewhere. However I don't think that a loose coupling based on name really improves the situation. You'd still need all the same code to make sure the particle system is copied along with the modifier, and new code will be needed to keep the link valid when the particle system is renamed. It makes things safer against crashes, but does not really improve the architecture in my opinion. I suggest putting the code from [D9537: Copy a single modifier to all selected objects.](https://archive.blender.org/developer/D9537) into a function that can be reused for the outliner, and ensure we get the design right when a new particle system is added.

Brecht Van Lommel commented

2021-01-18 14:34:49 +01:00

CoW should work correctly by the way, since BKE_object_copy_particlesystems updates the pointers.

CoW should work correctly by the way, since `BKE_object_copy_particlesystems` updates the pointers.

Bastien Montagne commented

2021-01-18 15:23:16 +01:00

In #82945#1095666, @brecht wrote:
CoW should work correctly by the way, since BKE_object_copy_particlesystems updates the pointers.

Huuuu yes indeed.... this is soooo fragile. Will at least add proper comments in code about this exception handling :(

In #82945#1095665, @brecht wrote:
I suggest putting the code from D9537: Copy a single modifier to all selected objects. into a function that can be reused for the outliner, and ensure we get the design right when a new particle system is added.

OK, sounds like a reasonable workaround/temp solution.

> In #82945#1095666, @brecht wrote: > CoW should work correctly by the way, since `BKE_object_copy_particlesystems` updates the pointers. Huuuu yes indeed.... this is soooo fragile. Will at least add proper comments in code about this exception handling :( > In #82945#1095665, @brecht wrote: > I suggest putting the code from [D9537: Copy a single modifier to all selected objects.](https://archive.blender.org/developer/D9537) into a function that can be reused for the outliner, and ensure we get the design right when a new particle system is added. OK, sounds like a reasonable workaround/temp solution.

blender-admin commented

2021-01-19 16:02:25 +01:00

This issue was referenced by c2e6969c56

This issue was referenced by c2e6969c56ef08bf1e7d831a9bfafcd36078eb3f

blender-admin commented

2021-01-19 16:02:25 +01:00

This issue was referenced by bc95c249a7

This issue was referenced by bc95c249a76563bffd8ce9242f7de6849cebe801

Bastien Montagne commented

2021-01-19 18:50:45 +01:00

Changed status from 'Confirmed' to: 'Resolved'

Bastien Montagne closed this issue

2021-01-19 18:50:45 +01:00

Thomas Dinges added this to the 2.92 milestone 2023-02-08 16:15:42 +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

Crash when dragging modifiers in Outliner #82945