Details

Reviewers

Commits

rBSed23bd5f3c51: Modifiers: ported Boolean DerivedMesh → Mesh
rBed23bd5f3c51: Modifiers: ported Boolean DerivedMesh → Mesh

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

Branch

mod_boolean_to2.8 (branched from blender2.8)

Build Status

Buildable 1499
Build 1499: arc lint + arc unit

Event Timeline

Christian Hubert (alikendarfen) created this revision.May 8 2018, 10:04 AM

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.

Christian Hubert (alikendarfen) updated this revision to Diff 10619.May 8 2018, 5:00 PM

Sybren A. Stüvel (sybren) added inline comments.May 8 2018, 5:37 PM

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.
305	Use `BKE_bmesh_to_mesh_nomain()` instead, it performs both these lines in one.

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

In D3236#76414, @Christian Hubert (alikendarfen) wrote:

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
305	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.
305	PS: I'll do that too while applying your patch.

Sybren A. Stüvel (sybren) added inline comments.May 9 2018, 10:05 AM

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:

modifier-boolean.blend398 KBDownload

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

Expanded BMALLOC_TEMPLATE_FROM_ME to accept two meshes
API change

Harbormaster completed remote builds in B1517: Diff 10730.May 9 2018, 3:40 PM

Some final formatting tweaks

This revision was not accepted when it landed; it landed in state Needs Review.May 9 2018, 3:50 PM

Closed by commit rBed23bd5f3c51: Modifiers: ported Boolean DerivedMesh → Mesh (authored by Christian Hubert (alikendarfen), committed by Sybren A. Stüvel (sybren)). · Explain Why

This revision was automatically updated to reflect the committed changes.

Sybren A. Stüvel (sybren) mentioned this in T54737: Modifiers: Convert from DerivedMesh → Mesh.May 9 2018, 3:51 PM

Boolean modifier port to 2.8
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 10627

source/blender/modifiers/intern/MOD_boolean.c

Boolean modifier port to 2.8ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 10627

source/blender/modifiers/intern/MOD_boolean.c

Boolean modifier port to 2.8
ClosedPublic
Actions

Revision Contents
Changeset List