Sculpt mode with Mask modifier enabled result an instant crash #47170

New Issue

Mohammad Eideh · 2016-01-12T18:01:29+01:00

Mohammad Eideh commented

2016-01-12 18:01:29 +01:00

Note, crash is resolved, but normal calculation logic looks like it may not be correct still. (though it doesn't result in visible errors, just extra calculations).

System Information
Win 7 64 bit

Blender Version
Broken: 2.76b official and build c986e93

As the title says; If you add a Mask modifier to a mesh (with a vertex group assigned), then switch to Sculpt mode...Blender will always crash for me. If you disable the modifier, switch to Sculpt mode and then re-enable the modifier...it'll crash once you start sculpting.

Exact steps for others to reproduce the error
mask.blend: Just switch to Sculpt mode .

*Note, crash is resolved, but normal calculation logic looks like it may not be correct still. (though it doesn't result in visible errors, just extra calculations).* ---- **System Information** Win 7 64 bit **Blender Version** Broken: 2.76b official and build c986e93 As the title says; If you add a Mask modifier to a mesh (with a vertex group assigned), then switch to Sculpt mode...Blender will always crash for me. If you disable the modifier, switch to Sculpt mode and then re-enable the modifier...it'll crash once you start sculpting. **Exact steps for others to reproduce the error** [mask.blend](https://archive.blender.org/developer/F273188/mask.blend): Just switch to Sculpt mode .

Mohammad Eideh commented

2016-01-12 18:01:29 +01:00

Changed status to: 'Open'

Mohammad Eideh commented

2016-01-12 18:01:29 +01:00

Added subscriber: @quacker

Campbell Barton commented

2016-01-12 19:59:23 +01:00

Added subscriber: @ideasman42

Campbell Barton commented

2016-01-12 19:59:23 +01:00

Caused by 4d33c37c9e

Caused by 4d33c37c9e

Bastien Montagne commented

2016-01-12 20:32:00 +01:00

Added subscriber: @mont29

Bastien Montagne commented

2016-01-12 20:32:00 +01:00

Crash confirmed, will have a look later.

Bastien Montagne self-assigned this 2016-01-12 20:32:16 +01:00

Campbell Barton commented

2016-01-12 22:20:24 +01:00

Added subscriber: @Psy-Fi

Campbell Barton commented

2016-01-12 22:20:24 +01:00

Looked into some different fixes.

At first I looked into passing mapping data to BKE_pbvh_update (pass polygon CD_ORIGINDEX and dm->numPolyData), then use this to array to only fill in the subset of faces used in the final CDDM (needs the reverse of the orig-index array). In this report the orig-index data isn't available (although it could be required and made available if it was important).

On further investigation I couldn't see why this call is even necessary, commenting all calls to cdDM_update_normals_from_pbvh, works well on my tests, with this report and with the file from #46726.

@Psy-Fi, any hints on what case this is needed for?

Assuming I missed some test case and this call is needed:

This is a possible fix: P313: only update normals on deform derived meshes since constructive modifiers wont have aligned arrays.

Further issues

When testing this report, I noticed you can sculpt onto invisible parts of the mesh (not a new bug, checked 2.71, 2.75 and 2.76 releases).
cdDM_update_normals_from_pbvh makes BKE_pbvh_update is running every redraw, where its mostly finding 0 nodes, yet it still runs pbvh_update_normals and allocates vertex array... which is never used (simple to return when totnode == 0, but probably best this bug gets sorted out first).

Looked into some different fixes. At first I looked into passing mapping data to `BKE_pbvh_update` (pass polygon `CD_ORIGINDEX` and `dm->numPolyData`), then use this to array to only fill in the subset of faces used in the final CDDM (needs the reverse of the orig-index array). In this report the orig-index data isn't available (although it could be required and made available if it was important). ---- On further investigation I couldn't see why this call is even necessary, commenting all calls to `cdDM_update_normals_from_pbvh`, works well on my tests, with this report and with the file from #46726. @Psy-Fi, any hints on what case this is needed for? ---- *Assuming I missed some test case and this call is needed:* This is a possible fix: [P313](https://archive.blender.org/developer/P313.txt): only update normals on deform derived meshes since constructive modifiers wont have aligned arrays. ---- Further issues ---- - When testing this report, I noticed you can sculpt onto invisible parts of the mesh *(not a new bug, checked 2.71, 2.75 and 2.76 releases)*. - `cdDM_update_normals_from_pbvh` makes `BKE_pbvh_update` is running every redraw, where its mostly finding 0 nodes, yet it still runs `pbvh_update_normals` and allocates vertex array... which is never used (simple to return when `totnode == 0`, but probably best this bug gets sorted out first).

Antonis Ryakiotakis commented

2016-01-13 00:34:49 +01:00

cdDM_update_normals_from_pbvh makes sure normals are updated even when we don't use optimal drawing from pbvh. It basically flushes the changes from PBVH to the derivedmesh itself. It's mostly useful on non-solid draw mode or when we can't do optimized drawing with PBVH (check can_pbvh_draw function)

Antonis Ryakiotakis commented

2016-01-13 00:38:09 +01:00

Note - if you comment this out, you'll probably be able to sculpt normally but your normals will be wrong in non-optimal drawing. The change is very subtle, you might miss the error unless you expect it, it should be apparent there. Try GLSL mode it should appear there.

blender-admin commented

2016-01-14 00:08:29 +01:00

This issue was referenced by 3e0f117ef5

This issue was referenced by 3e0f117ef574e1bbab5793db6cfe3fc8bd0fdce9

Campbell Barton commented

2016-01-14 00:16:27 +01:00

Changed status from 'Open' to: 'Resolved'

Campbell Barton closed this issue

2016-01-14 00:16:27 +01:00

Campbell Barton commented

2016-01-14 00:43:48 +01:00

@Psy-Fi, to fix the crash, committed check that the derived mesh isn't using a constructive modifier (it doesn't make any sense to copy normals between the arrays in that case - its random luck if their aligned or not).

Regarding your last comments, I think I see what you mean in principle, but I still can't see any shading difference with GLSL+Sculpt (matcaps / flat / smooth / with/without autosmooth ...).

When you have a modifier applied, the modifier stack will calculate the derived-mesh normals.

In the file from this report, for example - its calling CDDM_calc_normals_mapping_ex,
in fact with a deform modifier, with and without autosmooth, its calculating normals in sculpt mode, see call stack: P314.

(IIRC auto-smooth normals were meant to be disabled while sculpting).

@Psy-Fi, to fix the crash, committed check that the derived mesh isn't using a constructive modifier *(it doesn't make any sense to copy normals between the arrays in that case - its random luck if their aligned or not)*. Regarding your last comments, I think I see what you mean in principle, but I still can't see any shading difference with GLSL+Sculpt (matcaps / flat / smooth / with/without autosmooth ...). When you have a modifier applied, the modifier stack will calculate the derived-mesh normals. In the file from this report, for example - its calling `CDDM_calc_normals_mapping_ex`, in fact with a deform modifier, with and without autosmooth, its calculating normals in sculpt mode, see call stack: [P314](https://archive.blender.org/developer/P314.txt). *(IIRC auto-smooth normals were meant to be disabled while sculpting).*

Campbell Barton commented

2016-01-14 00:47:44 +01:00

Changed status from 'Resolved' to: 'Open'

Campbell Barton reopened this issue

2016-01-14 00:47:44 +01:00

Campbell Barton commented

2016-01-14 00:47:44 +01:00

Re-opening this report, since there are some unresolved issues with normal calculation, (even though the crash is fixed).

Re-opening this report, since there are some unresolved issues with normal calculation, *(even though the crash is fixed).*

Bastien Montagne removed their assignment 2016-01-14 10:31:29 +01:00

Campbell Barton was assigned by Bastien Montagne

2016-01-14 10:31:29 +01:00