Paths

Table of Contentst

Fix T70006: crash when transforming paint curve edit points in sculpt mode
ClosedPublic
Actions

Authored by Philipp Oeser (lichtwerk) on Sep 18 2019, 1:33 PM.

Details

Reviewers

Pablo Dobarro (pablodp606)
Jeroen Bakker (jbakker)
Germano Cavalcante (mano-wii)
Brecht Van Lommel (brecht)

Maniphest Tasks

T70006: Crash when using Curve as Stroke Method for Crease or Pinch in sculpt mode

Commits

rB67310ed97618: Fix T70006: crash when transforming paint curve edit points in sculpt mode

Summary

This was caused by rB309cd047ef46.
Above commit introduced code that would skip early for sculptmode
(leaving out the neccessary createTransPaintCurveVerts).

Diff Detail

Repository

rB Blender

Branch

T70006 (branched from master)

Build Status

Buildable 5007
Build 5007: arc lint + arc unit

Event Timeline

Philipp Oeser (lichtwerk) created this revision.Sep 18 2019, 1:33 PM

Harbormaster completed remote builds in B5007: Diff 18317.Sep 18 2019, 1:33 PM

Interesting to see that CTX_SCULPT and CTX_PAINT_CURVE can be set at the same time.
This may however be hiding some other bugs.
In addition to this patch, I propose to create a new flag (T_SCULPT) for t->flag and replace other possible errors due to testing with t->options & CTX_SCULPT.

Not sure if we should mix the fix with the refactor?
Also have to make myself a little more familiar with the distinction between t->options and t->flag (not quite clear to me yet...)

@Pablo Dobarro (pablodp606) : would you be wanting to do the refactor?

I'm setting CTX_SCULPT in initTransform checking if the active object is in sculpt mode. I can make the refactor in a separate patch. I'm also not sure about how t->options and t->flags should be used, but in the future I would like to add another option to transform only the pivot point and to use the custom pivot point orientation for the transform. I was using CTX_SCULPT and CTX_SCULPT_PIVOT in the sculpt branch.

In D5837#134070, @Philipp Oeser (lichtwerk) wrote:

(...) have to make myself a little more familiar with the distinction between t->options and t->flag (not quite clear to me yet...)

t->flag & T_SCULPT would mean that you are transforming an object of "type" "SCULPT".
t->options & CTX_SCULPT means you are in the SCULPT context.

It is possible to have more than one context per operation.
But it should not be possible to transform more than one "type" of transform object.

The problem is that createTransData, recalcData, special_aftertrans_update, all have slightly different logic for determining the data being transformed.

The simple fix here is to reorder else if (t->options & CTX_SCULPT) { to be in the same place for all, after CTX_PAINT_CURVE.

For refactoring beyond this fix, really there should be a transform context enum, that contains all types of contexts and has mutually exclusive items. Every transform_convert_*.c file should contain the functions for a specific context, for creating the data, updating during transform, and updating at the end of transform.

The simple fix here is to reorder else if (t->options & CTX_SCULPT) { to be in the same place for all, after CTX_PAINT_CURVE.

not sure if this really makes it prettier?

P1105 D5837_b

1
2
3	diff --git a/source/blender/editors/transform/transform_convert.c b/source/blender/editors/transform/transform_convert.c
4	index 1e783e0e7b8..ef11e2c7aed 100644
5	--- a/source/blender/editors/transform/transform_convert.c
6	+++ b/source/blender/editors/transform/transform_convert.c
7	@@ -2399,10 +2399,6 @@ void createTransData(bContext C, TransInfo t)
8	}
9	countAndCleanTransDataContainer(t);
10	}
11	- else if (t->options & CTX_SCULPT) {
12	- createTransSculpt(t);
13	- countAndCleanTransDataContainer(t);
14	- }
15	else if (t->options & CTX_TEXTURE) {
16	t->flag \|= T_TEXTURE;
17	t->obedit_type = -1;
18	@@ -2683,10 +2679,14 @@ void createTransData(bContext C, TransInfo t)
19	createTransPaintCurveVerts(C, t);
20	countAndCleanTransDataContainer(t);
21	}
22	- else {
23	+ else if (!(t->options & CTX_SCULPT)){
24	has_transform_context = false;
25	}
26	}
27	+ else if (t->options & CTX_SCULPT) {
28	+ createTransSculpt(t);
29	+ countAndCleanTransDataContainer(t);
30	+ }
31	else if ((ob) &&
32	(ELEM(
33	ob->mode, OB_MODE_PAINT_GPENCIL, OB_MODE_SCULPT_GPENCIL, OB_MODE_WEIGHT_GPENCIL))) {

Since we already do this:
else if (ob && (ob->mode & OB_MODE_WEIGHT_PAINT) && !(t->options & CTX_PAINT_CURVE)) {
I think the patch in its current state is more in line and actually more readable?

Anyways,... just need a green light for any one of the two...
Thx for looking at this btw @Germano Cavalcante (mano-wii), @Pablo Dobarro (pablodp606), @Brecht Van Lommel (brecht) !

Seems it's a bigger mess than I thought. Just commit this as is then.

This revision is now accepted and ready to land.Sep 18 2019, 7:24 PM

Closed by commit rB67310ed97618: Fix T70006: crash when transforming paint curve edit points in sculpt mode (authored by Philipp Oeser (lichtwerk)). · Explain WhySep 18 2019, 7:32 PM

This revision was automatically updated to reflect the committed changes.

Philipp Oeser (lichtwerk) added a commit: rB67310ed97618: Fix T70006: crash when transforming paint curve edit points in sculpt mode.

What about this line. Will not have problem too?
https://developer.blender.org/diffusion/B/browse/master/source/blender/editors/transform/transform.c$2302

In D5837#134231, @Germano Cavalcante (mano-wii) wrote:

What about this line. Will not have problem too?
https://developer.blender.org/diffusion/B/browse/master/source/blender/editors/transform/transform.c$2302

Yeah, we should skip that as well [ should have paid attention to what you where saying :) ], thx for the heads up, will do in a sec...

Revision Contents
Changeset List

Path

Size

source/

blender/

editors/

transform/

transform_convert.c

2 lines

Diff 18317

View Options

source/blender/editors/transform/transform_convert.c

Fix T70006: crash when transforming paint curve edit points in sculpt modeClosedPublicActions