Paths

Table of Contentst

Fix T74888: Operators called within spline loops invalidate PointerRNA pointer
AbandonedPublic
Actions

Authored by Germano Cavalcante (mano-wii) on Mar 20 2020, 1:12 AM.

Details

Reviewers

Campbell Barton (campbellbarton)
Bastien Montagne (mont29)
Sergey Sharybin (sergey)

Maniphest Tasks

T74888: Python curve hook operators mess with selection (Probably only on Windows)

Summary

Usually Curve operators calls ED_curve_editnurb_load and ED_curve_editnurb_make which creates another cu->nurb and free the old one.
But the old cu->nurb may be being referenced by a PyObject (type pyrna_prop_collection_Type with pointer inited in rna_Curve_splines_begin).
This can lead to crash or other nasty problems since RNA_POINTER_INVALIDATE(nu_ptr) is not called.

The idea of this patch is not to free the cu->nurb and editnurb->nurbs pointers.

How this patch solves the problem:
Although not used, the BKE_nurbList_duplicate logic is duplicated in ED_curve_editnurb_load and ED_curve_editnurb_make (with minor differences).
Basically these functions copy the ListBase of nurbs.
But to copy, the old nurb is always freed and a new one is allocated.
This patch changes the behavior of BKE_nurbList_duplicate to reuse pointers, and deduplicates the code in` ED_curve_editnurb_load` and ED_curve_editnurb_make by using that BKE function.

Ref T74888

Diff Detail

Repository

rB Blender

Branch

master

Build Status

Buildable 7238
Build 7238: arc lint + arc unit

Event Timeline

Germano Cavalcante (mano-wii) created this revision.Mar 20 2020, 1:12 AM

Harbormaster completed remote builds in B7231: Diff 22961.Mar 20 2020, 1:12 AM

Sergey Sharybin (sergey) added inline comments.Mar 20 2020, 9:16 AM

source/blender/blenkernel/intern/curve.c
494–498	General note: this is too vague of a name. What exactly is happening in this function? When is it to be used? Name should be helping in such questions, and also worth adding comment.
source/blender/editors/curve/editcurve.c
1312	Are there cases where this branch is executed and there was something else between previous editnub_load and this editnurb_make?

Germano Cavalcante (mano-wii) added inline comments.Mar 20 2020, 12:22 PM

source/blender/blenkernel/intern/curve.c
494–498	This is a name that I have a hard time defining. The idea is to reuse what is already allocated (when possible). If there is no space, the member must be reallocated. My ideas for names: `BKE_nurb_compare_and_update` (remembers COW) `BKE_nurb_realloc_and_copy_data`
source/blender/editors/curve/editcurve.c
1312	Apparently not. This function is either called when there is no `editnurb` or it is called right after `ED_curve_editnurb_load` which makes this copy redundant (since `ED_curve_editnurb_load` copies from `cu->nurb` from `editnurb->nurbs` and this does the opposite).

In a general case is hard to keep iterators always valid. But here i think it's quite isolated and clear fix, and covers something which people are running into quite often.

So from me once the naming is figured out this is a nice fix to have.

source/blender/blenkernel/intern/curve.c
494–498	Doesn't seem that it compares anything than number of points and such. As in, it dones't compare coordinates. Think best is to make it non-public function and be something like: /* Copy content of nu_src to nu_dst without doing any unnecessary (re)allocations. * The destination container itself is expected to be allocated and is never touched here. * The points arrays and other fields will only be re-allocated if their size does not match. / static void nurbs_copy_inplace(const Nurb nu_src, Nurb *nu_dst) { } The reason to make it non-public is because it's quite obscure function, and there is no really need for it to be exposed.

Rename and comment BKE_nurb_update
make nurbs_copy_inplace a non-public and edit BKE_nurbList_duplicate instead
Use BKE_nurbList_duplicate to avoid free a nurbList

Harbormaster completed remote builds in B7238: Diff 22971.Mar 20 2020, 1:25 PM

Germano Cavalcante (mano-wii) edited the summary of this revision. (Show Details)Mar 23 2020, 1:28 PM

Am not really keen on this fix, mainly because it's limited to curves, the same problems remain for meshes.

In general, if we try to keep arrays valid between mode switching, it's going to complicate code and not work reliably - since in some cases the arrays size wont match.

Since so few operators flush edits back to the object data, it would be interesting to see if we could support adding hooks without writing edit-changes back to the original object-data, perhaps use the edit-data in place until edit-mode exists.

In general, if we try to keep arrays valid between mode switching, it's going to complicate code and not work reliably - since in some cases the arrays size wont match.

In general - yes. In a specific case it is possible to have it work reliably quite easily.
Sure, the limitation is that iteration need to happen through edit mode curves, otherwise you are in trouble.

Since so few operators flush edits back to the object data, it would be interesting to see if we could support adding hooks without writing edit-changes back to the original object-data, perhaps use the edit-data in place until edit-mode exists.

I'm not sure how would that work. Have separate vertex indices in hook modifier for edit mode? Change meaning of indices depending on object mode?
How to support rendered viewport next to edit mode one? How to support hook modifiers after constructive modifier?

In the original bug (reported), the problem is concentrated in the LinkList pointer.
So just reusing LinkList links would be enough to solve the problem.
Reusing the arrays is a bonus (I confess I should have done it separately).
On arrays, BMeshs solve this problem by maintaining a PyObject reference as a CustomData.
We could do something like that for curves but that would be a big change.

EDIT
Okay, after a second test, I realized that to fix the bug it is important to keep the reference of the arrays as well. (In addition to LinkList).

This is a cosmetic change for a specific case used for python scripts.
The real solution is much more engaging.

Revision Contents
Changeset List

Path

Size

source/

blender/

blenkernel/

intern/

curve.c

117 lines

editors/

curve/

editcurve.c

25 lines

Diff 22971

View Options

source/blender/blenkernel/intern/curve.c