Page MenuHome

Multi object editing - UV_OT_stitch - add state save and load
ClosedPublic

Authored by Alan Troth (Al) on Aug 24 2018, 10:35 PM.

Details

Summary

Multi object editing - UV_OT_stitch - add state save and load

Diff Detail

Repository
rB Blender
Branch
arcpatch-D3645 (branched from blender2.8)
Build Status
Buildable 2027
Build 2027: arc lint + arc unit

Event Timeline

Dalai Felinto (dfelinto) requested changes to this revision.Sep 4 2018, 8:15 PM

I get a crash with my test file:

  • Press V + Enter
  • Again, V + Enter

Now try to edit any of the redo settings.
BLI_assert failed: //source/blender/editors/uvedit/uvedit_smart_stitch.c:2244, stitch_init_all(), at 'sel_idx < total_selected'

source/blender/editors/uvedit/uvedit_smart_stitch.c
2253

Loose the * 1.

2362

Do you need to MEM_callocN it here? Can't you MEM_mallocN instead?

2405

To tag an object to geometry reconstruction shouldn't be done lightly. We should at least check if when synced_selection is true there are any selected elements (look at my recent UV commits I did it there a few times).

2728

Avoid abbreviations in the API, even when not exposed in the UI: objs > objects. You can even name it selection_count instead.

This revision now requires changes to proceed.Sep 4 2018, 8:15 PM

Attached a simpler test file for the crash previously mentioned:

source/blender/editors/uvedit/uvedit_smart_stitch.c
2261

Shouldn't this be +=?

I've found reason for the crash, I just need half an hour to fix it.

The collection that stores the selected UVs only gets cleared after it's been read, it should be cleared before it gets written. With the code as it is, 'V + Enter' will store a list of selected UVs, a second 'V + Enter' will add (append) the same selected vertices to the collection again.

Blender2.79 has the same problem. 2.79 doesn't have an assert that can fire and I think the worst that will happen with the duplicate UVs is that a UV will be selected more than once.

Alan Troth (Al) added inline comments.Sep 6 2018, 9:01 PM
source/blender/editors/uvedit/uvedit_smart_stitch.c
2261

It should be =.

state_init->uv_selected_count (objs_selection_count[ob_index]) tells how many UVs to select (how many to read from state_init->to_select) when StitchState is initialised.

Alan Troth (Al) updated this revision to Diff 11791.EditedSep 6 2018, 11:37 PM
  • Crash fix.
  • Addressed changes to revision.
Alan Troth (Al) marked 4 inline comments as done.Sep 6 2018, 11:39 PM
Alan Troth (Al) added inline comments.
source/blender/editors/uvedit/uvedit_smart_stitch.c
2335

Should there also be a DEG_id_tag_update(obedit->data, 0); here ?

Embracing multi-objects a bit further:

At the moment uvs can be stitched together if they occupy the same point in 3D space and if they are part of the same object.

Embracing multi-objects a bit further, I think that uvs that occupy the same point in 3D space but are from different objects should also have the potential to be stitched.

  • Put back MEM_SAFE_FREE as these pointer might be NULL when trying to free.

Comitted on rB601fd9683cf59a789f0bbf2157724f3051428da2.

There are things I'm still not happy about this operator though. But I think part of it comes from your other stitch patch.
For one code style (lines are longer than 120). But mainly the fact that we over tag objects for update. This is not cheap, and shouldn't be done lightly.

Embracing multi-objects a bit further, I think that uvs that occupy the same point in 3D space but are from different objects should also have the potential to be stitched.

That would be a nice addition, yes,

This revision is now accepted and ready to land.Sep 7 2018, 12:10 PM