Page MenuHome

Boolean modifier port to 2.8
ClosedPublic

Authored by Christian Hubert (alikendarfen) on May 8 2018, 10:04 AM.

Details

Summary

An issue: when click on "apply", the mesh comes back to its original (not modified) state.

The reason is the call to "get_mesh_eval_for_modifier(bmd->object, ctx->flag);" (which retrieves the linked object in line 178), because in the context of "apply" and only in this context, "ob->mesh_evaluated" is null, so no boolean operation is calculated after that.

I was not able to compare the call stacks (the one when the modifier is just calculated and the one when it is applied), as there is too many things I do not know here.

Diff Detail

Repository
rB Blender

Event Timeline

Additional info:

  • With array modifier if an 'object offset' is set, it is ignored when 'apply' button is clicked (so there is a problem for array too). You can also try "mirror": same thing
  • For boolean, if you apply the modifier (with a linked object), it is ignored (as mentioned previously) but additionally if "ctrl+Z", the linked object if not set back in the modifier (this is not the case for array or mirror).

The "BKE_object_eval_uber_data" from "object_update.c" is not called for the linked object when the modifier is applied (this function calculates "mesh_evaluated" if "DEG_depsgraph_use_copy_on_write()", and the issue is due to a null "mesh_evaluated".

This function is called asynchronously, so I cannot use the stack trace to go further. Where to search to find the setup of the thread pool for it?

Also, I'm not able to find where the "depsgraph" is used to check the modified object dependencies. Do you have some info about that?

Sybren A. Stüvel (sybren) requested changes to this revision.May 8 2018, 2:22 PM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/modifiers/intern/MOD_boolean.c
124

Use BKE_mesh_new_nomain instead.

142

See comment below, mark vertex normals dirty.

173

Try to keep the port as straight-forward as possible. Right now the patch reads as if you removed dm_other altogether, because the declaration was moved.

317

Marking normals as dirty shouldn't be removed, but ported too. Use result->runtime.cd_dirty_vert |= CD_MASK_NORMAL;

This revision now requires changes to proceed.May 8 2018, 2:22 PM

If I understood @Sergey Sharybin (sergey) correctly, we shouldn't be worrying about the 'apply' not working correctly. It's a copy-on-write / depsgraph issue that'll be solved later.

source/blender/modifiers/intern/MOD_boolean.c
63

Don't include unnecessary formatting please.

122

DM_from_template translates to BKE_mesh_from_template, whereas CDDM_copy here would probably translate better to BKE_id_copy_ex(NULL, ob->data, (ID **)&mesh, LIB_ID_CREATE_NO_MAIN | LIB_ID_CREATE_NO_USER_REFCOUNT | LIB_ID_CREATE_NO_DEG_TAG, false);

182

Same here; don't move declarations around if you don't have to, it makes the diff harder to understand and harder to merge.

306

Use BKE_bmesh_to_mesh_nomain() instead, it performs both these lines in one.

Christian Hubert (alikendarfen) marked 4 inline comments as done.
  • Removed blank between includes
  • BKE_id_copy_ex instead of BKE_mesh_from_template
  • Don't move declaration: BMesh *bm;
  • BKE_bmesh_to_mesh_nomain instead of BKE_mesh_new_nomain and BM_mesh_bm_to_me
  • Removed blank between includes

As I said on IRC: that's not what I meant. I meant not to change any formatting (which includes empty lines) in this diff, unless you have to. So don't change any of the newlines, it only causes noise. I'll update this while applying your patch, it's just something to keep in mind for other patches.

source/blender/modifiers/intern/MOD_boolean.c
306

Formatting: the rest of the code uses &((struct BMeshToMeshParams){0}) (so without spaces around/in the {0}), so for consistency you could use that too.

306

PS: I'll do that too while applying your patch.

source/blender/modifiers/intern/MOD_boolean.c
122

This doesn't even compile. BKE_id_copy_ex() returns a bool, not a Mesh *. The code I pasted here was just an example, not something that could be blindly copied in.

After fixing the compiler error I still get bad results. Expected:

Actual:

This is with this file, started with blender --enable-copy-on-write modifier-boolean.blend:

Christian Hubert (alikendarfen) marked 2 inline comments as done.
  • Corrected call to BKE_id_copy_ex (occurred for empty meshes and was not tested before)
  • Corrected the order in which meshes are pushed to the bmesh (explains the problem with icosphere)
  • Tested on more complex meshes appended (shift+F1), results seem similar (see linked images)

But more globally, it is hard to see if the results are really correct as the boolean result cannot be viewed (meshes overlap) and the modifier cannot be applied.

It doesn't compile on my machine:

/home/sybren/workspace/blender-git/blender/source/blender/modifiers/intern/MOD_boolean.c: In function ‘applyModifier’:
/home/sybren/workspace/blender-git/blender/source/blender/modifiers/intern/MOD_boolean.c:186:79: error: macro "BMALLOC_TEMPLATE_FROM_ME" passed 2 arguments, but takes just 1
    const BMAllocTemplate allocsize = BMALLOC_TEMPLATE_FROM_ME(mesh, mesh_other);
                                                                               ^
/home/sybren/workspace/blender-git/blender/source/blender/modifiers/intern/MOD_boolean.c:186:38: error: ‘BMALLOC_TEMPLATE_FROM_ME’ undeclared (first use in this function)
    const BMAllocTemplate allocsize = BMALLOC_TEMPLATE_FROM_ME(mesh, mesh_other);
                                      ^~~~~~~~~~~~~~~~~~~~~~~~
/home/sybren/workspace/blender-git/blender/source/blender/modifiers/intern/MOD_boolean.c:186:38: note: each undeclared identifier is reported only once for each function it appears in
Sybren A. Stüvel (sybren) edited the summary of this revision. (Show Details)
  • Expanded BMALLOC_TEMPLATE_FROM_ME to accept two meshes
  • API change
  • Some final formatting tweaks
This revision was not accepted when it landed; it landed in state Needs Review.May 9 2018, 3:50 PM
This revision was automatically updated to reflect the committed changes.