Improve Mirror Modifier clipping behaviour with constraint translation axis
Needs ReviewPublic
Actions

Authored by Henrik Dick (weasel) on May 1 2020, 12:03 PM.

Details

Reviewers

Group Reviewers

Summary

This patch makes sure that if you translate/scale something in editmode with mirror clipping, it will stay on it's constraint axis/plane even if that doesn't align with the clipping plane normal. This is especially useful when using multiple rotated mirror modifiers.
The behaviour for non constraint translation/scaling is unchanged.

Here is one very useful example of the new behaviour:

Here is one example where the clipping behaviour is changed. The already clipped vertices will stay in place in this case.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Henrik Dick (weasel) requested review of this revision.May 1 2020, 12:03 PM

Rebase
fix one more corner case
fix error when clipping in multiple planes

Rebase

I tried a lot of cases and noticed, that there is one case where the current (in master and in this patch) is strange. If you have a vertex at the intersection of two clipping planes it can slip through or rip lose because of the way multiple clipping planes are handled. What would be the desired behaviour? Should such a vertex just be locked to the intersection line?

Rebase

Any feedback on this?

The idea is good, but I believe that the patch can be simplified.
For example this code:

(t->con.mode & CON_AXIS0) | (t->con.mode & CON_AXIS1) |
                                         (t->con.mode & CON_AXIS2)

could be replaced by:

(t->con.mode & (CON_AXIS0 | CON_AXIS1 | CON_AXIS2))

And the entire new logic below could be simplified with a projection matrix.

Or using the `isect_ray_plane_v3` function.

Rebase
simplification
fixed all edge cases that I found so far

Germano Cavalcante (mano-wii) added a comment.Jun 13 2020, 5:10 AM

This comment was removed by Germano Cavalcante (mano-wii).

@Germano Cavalcante (mano-wii) I tried your patch. I think you missed something because it did weird things, but at least I can see where you were going codewise. I will try to rethink some parts of my code and see where it brings me.
I also included a test file for testing clipping in various advanced ways. With my current patch it at least works in every case that I have seen yet (I may or may not have missed something).

Mirrorclippingtest.blend766 KBDownload

minor changes and simplification

I kind of think that this might be already at the minimum of what is neccessary to make clipping work reliable, stable and predictable. There are a couple lines where one could argue to move them in or out of a loop, but for general geometry (the main part not being clipped) this should be good.
I don't think that there are really big improvements possible when it comes to simplification from here on, because you have to consider all the rotated clipping planes. In the file that I provided you can test a lot of things where most approaches would fail to work, because they make assumtions about the alignment or position of the clipping planes.
The code could be rewrote to use projection matrices everywhere instead of actually projecting each vertex, but I don't think that the efficiency would be significantly different, nor the readability.

In D7592#194302, @Henrik Dick (weasel) wrote:

@Germano Cavalcante (mano-wii) I tried your patch. I think you missed something because it did weird things...

Okay, I fixed the bugs and limited the change to contraint only:

P1471 Patch for D7592

index

---

+++

100

101

102

103

104

105

106

107

108

109

110

111

112

113

114

115

116

117

118

119

120

121

122

123

124

125

126

127

128

129

diff --git a/source/blender/editors/transform/transform_convert.c b/source/blender/editors/transform/transform_convert.c 8496642185d..92d3eb974f2 100644 a/source/blender/editors/transform/transform_convert.c b/source/blender/editors/transform/transform_convert.c -1342,43 +1342,54 @@ void clipMirrorModifier(TransInfo *t) FOREACH_TRANS_DATA_CONTAINER (t, tc) { Object *ob = tc->obedit; ModifierData *md = ob->modifiers.first; float tolerance[3] = {0.0f, 0.0f, 0.0f}; int axis = 0; bool is_constraint = (t->con.mode & CON_APPLY) != 0; for (; md; md = md->next) { if ((md->type == eModifierType_Mirror) && (md->mode & eModifierMode_Realtime)) { MirrorModifierData *mmd = (MirrorModifierData *)md; if (mmd->flag & MOD_MIR_CLIPPING) { axis = 0; int axis = 0; if (mmd->flag & MOD_MIR_AXIS_X) { axis |= 1; tolerance[0] = mmd->tolerance; } if (mmd->flag & MOD_MIR_AXIS_Y) { axis |= 2; tolerance[1] = mmd->tolerance; } if (mmd->flag & MOD_MIR_AXIS_Z) { axis |= 4; tolerance[2] = mmd->tolerance; } if (axis) { float mtx[4][4], imtx[4][4]; int i; if (axis) { float mirror_plane[3][4]; int mirror_plane_len = 0; float(*mirror_mtx)[4] = NULL; if (mmd->mirror_ob) { float obinv[4][4]; mirror_mtx = mmd->mirror_ob->obmat; } else if (tc->use_local_mat) { mirror_mtx = tc->mat; } invert_m4_m4(obinv, mmd->mirror_ob->obmat); mul_m4_m4m4(mtx, obinv, ob->obmat); invert_m4_m4(imtx, mtx); for (int i = 0; i < 3; i++) { if (axis & (1 << i)) { float *mirror_pl = mirror_plane[mirror_plane_len++]; if (mirror_mtx) { plane_from_point_normal_v3(mirror_pl, mirror_mtx[3], mirror_mtx[i]); mirror_pl[3] /= normalize_v3(mirror_pl); } else { zero_v4(mirror_pl); mirror_pl[i] = 1.0f; } } } float tolerance = mmd->tolerance; TransData *td = tc->data; for (i = 0; i < tc->data_len; i++, td++) { int clip; for (int i = 0; i < tc->data_len; i++, td++) { float loc[3], iloc[3]; bool clip = false; if (td->loc == NULL) { break; -1391,34 +1402,32 @@ void clipMirrorModifier(TransInfo *t) copy_v3_v3(loc, td->loc); copy_v3_v3(iloc, td->iloc); if (mmd->mirror_ob) { mul_m4_v3(mtx, loc); mul_m4_v3(mtx, iloc); if (tc->use_local_mat) { mul_m4_v3(tc->mat, loc); mul_m4_v3(tc->mat, iloc); } clip = 0; if (axis & 1) { if (fabsf(iloc[0]) <= tolerance[0] || loc[0] * iloc[0] < 0.0f) { loc[0] = 0.0f; clip = 1; for (int j = 0; j < mirror_plane_len; j++) { float t_iloc = plane_point_side_v3(mirror_plane[j], iloc); float t_loc = plane_point_side_v3(mirror_plane[j], loc); if (fabs(t_iloc) <= tolerance || (t_loc * t_iloc) < 0.0f) { clip = true; if (is_constraint) { /* Intersect movement with the mirror plane. */ float move_dist = (t_iloc - t_loc); if (move_dist) { interp_v3_v3v3(loc, iloc, loc, t_iloc / move_dist); continue; } } /* Project the location on the mirror plane. */ madd_v3_v3fl(loc, mirror_plane[j], -t_loc); } } if (axis & 2) { if (fabsf(iloc[1]) <= tolerance[1] || loc[1] * iloc[1] < 0.0f) { loc[1] = 0.0f; clip = 1; } } if (axis & 4) { if (fabsf(iloc[2]) <= tolerance[2] || loc[2] * iloc[2] < 0.0f) { loc[2] = 0.0f; clip = 1; } } if (clip) { if (mmd->mirror_ob) { mul_m4_v3(imtx, loc); if (tc->use_local_mat) { mul_m4_v3(tc->imat, loc); } copy_v3_v3(td->loc, loc); }

If I didn't get it wrong, this patch does exactly the same thing as the one proposed here.
But it is much smaller.

Sorry to disagree but did you try moving vertices in a plane (like with Shift+Z)? It is completely different in that case. And then also consider moving it in any arbitrary constraint plane (like its commonly the case when using Normal Space).

In D7592#194446, @Henrik Dick (weasel) wrote:

Sorry to disagree but did you try moving vertices in a plane (like with Shift+Z)? It is completely different in that case. And then also consider moving it in any arbitrary constraint plane (like its commonly the case when using Normal Space).

So, the proposal here is not clear (See https://wiki.blender.org/wiki/Tools/CodeReview)
Here's what the patch I posted does. What seems useful (GIF):

Ok I'm very sorry. Let me explain again. I wrote, I want that if I specify a constraint then the movement should always be constraint by that constraint. Clipping was always shifting vertices out of the constraint movement. So if I move something on a line, it should stay on that line, even when clipping. If I move something on a plane, it should stay on that plane, no matter what. Also I wanted to keep all the good behaviours of the old clipping algorithm. Especially when combined with snapping. So I will add three more gifs to show the described behaviour.

Moving on a plane works in the simplest case just like before

Moving and snapping on a plane works like before

moving on a tilted plane clips perfectly like expected

move if condition fixing a tiny bug that I found when making the gifs
fix alternative plane constraint code

All these gifs show the same situation that is seen in the second patch that I attached (patch that has less lines of changes, is more efficient and, in my opinion, more readable).
Allocating and freeing LinkList elements in each interaction is not attractive. It is inefficient and seems to complicate more than help. Is it really necessary?

Ok maybe I am doing something stupid, but If I apply your patch and try to reproduce the first gif then it looks like this:

which is not so much desirable.

The allocation of the link list is there to reduce the number of times that the nesseccary matrices are multiplied and inverted by (3*vertexcount) times so it's not strictly nesseccary but should help with performance.

EDIT: (a little late)
I also struggle to understand what these lines in your code do (could you explain briefly?)

-                if (mmd->mirror_ob) {
-                  mul_m4_v3(imtx, loc);
+                if (tc->use_local_mat) {
+                  mul_m4_v3(tc->imat, loc);

I tested the other gifs. But in fact the first one shows this behavior.
Checking...

also the second gif does not work with your patch for the same reason as the first one when using a plane constraint. The third gif shows very weird results with your patch, which are also different to my patch (which I do understand why, but that is not desirable IMO)

fix stupid typo in last revision

Germano Cavalcante (mano-wii) added inline comments.Jun 15 2020, 7:02 PM

source/blender/editors/transform/transform_convert.c
1338	By blender convention, usually variables of type bool start with is_, use_, has_. So a better name would be `use_constraint_correction` or similar.
1343	These variables can be moved out of the loop.
1345	Is there any way to avoid this ListBase?
1397	This variable does not change, it can be out of the main loop.
1401	`const_plane` is a confusing name. (const looks like constant). You can use the `getConstraintSpaceDimension` function to find out the constraint type. It is also a variable that can be out of the main loop.
1426	Do not use these preprocessing directives (#if 1, #else, #endif). If something needs to be better explained, add a comment.

Germano Cavalcante (mano-wii) requested changes to this revision.Jun 15 2020, 7:03 PM

This revision now requires changes to proceed.Jun 15 2020, 7:03 PM

made requested changes

The ListBase is nesseccary, because of the changing constraint space. If a vertex is clipped while using a constraint plane, then it should stay on the intersection line in the following clipping procedures. To make that possible it is nesseccary to store this data temporarily per vertex/TransData. I decided to instead of storing that information per vertex to swap iteration order and store the other expensive calculations of the space matrices in a list. The list is usually one or two elements long so I really don't see a problem. For the most part this will result in just a couple mallocs per frame, with a size independent of the vertex count. If you compare that to any modifier it's obvious, that it will have a neglegtible effect on the performance and memory consumption.

EDIT:
I did some simple performance testing on a dense mesh and did not notice a real performance difference between 2.83 and my patch here. (my testing was really simple, so I could be very wrong in the more general case)

Although it works, I am not comfortable with this solution.
I would prefer to work with planes v4 in the global space instead of this series of matrices (mtx, imtx, obmat, obinv).
And although this solution is located in a generic utility file, it does not work for rotation and bend.

To avoid ListBase, how about using a utility like this?

typedef struct MirrorClippingData {
  float plane[4];
  float tolerance;
} MirrorClippingData;

static MirrorClippingData g_mirror_data_stack[3];

MirrorClippingData *trans_mirror_data_create(const TransDataContainer *tc,
                                             int *r_len,
                                             bool *r_is_alloced)
{
  MirrorClippingData *mirror_data = g_mirror_data_stack;
  int mirror_data_len = 0;
  int mirror_data_len_alloced = ARRAY_SIZE(g_mirror_data_stack);
  bool mirror_data_is_alloced = false;

  const Object *ob = tc->obedit;
  const ModifierData *md = ob->modifiers.first;

  for (; md; md = md->next) {
    if ((md->type == eModifierType_Mirror) && (md->mode & eModifierMode_Realtime)) {
      const MirrorModifierData *mmd = (MirrorModifierData *)md;

      if (mmd->flag & MOD_MIR_CLIPPING) {
        int axis = (mmd->flag & MOD_MIR_AXIS_X) ? 1 : 0;
        axis |= (mmd->flag & MOD_MIR_AXIS_Y) ? 2 : 0;
        axis |= (mmd->flag & MOD_MIR_AXIS_Z) ? 4 : 0;

        if (axis) {
          const float(*mirror_mtx)[4] = NULL;
          if (mmd->mirror_ob) {
            float obinv[4][4];
            mirror_mtx = mmd->mirror_ob->obmat;
          }
          else if (tc->use_local_mat) {
            mirror_mtx = tc->mat;
          }

          for (int i = 0; i < 3; i++) {
            if (axis & (1 << i)) {
              if (mirror_data_len == mirror_data_len_alloced) {
                mirror_data_len_alloced *= 2;
                MirrorClippingData *mdata;
                mdata = MEM_mallocN(sizeof(*mdata) * mirror_data_len_alloced, __func__);
                memcpy(mdata, mirror_data, sizeof(*mdata) * mirror_data_len);
                if (mirror_data_is_alloced) {
                  MEM_freeN(mirror_data);
                }
                mirror_data = mdata;
                mirror_data_is_alloced = true;
              }

              MirrorClippingData *md_new = &mirror_data[mirror_data_len++];
              md_new->tolerance = mmd->tolerance;
              if (mirror_mtx) {
                plane_from_point_normal_v3(md_new->plane, mirror_mtx[3], mirror_mtx[i]);
                md_new->plane[3] /= normalize_v3(md_new->plane);
              }
              else {
                zero_v4(md_new->plane);
                md_new->plane[i] = 1.0f;
              }
            }
          }
        }
      }
    }
  }

  *r_len = mirror_data_len;
  *r_is_alloced = mirror_data_is_alloced;
  return mirror_data;
}

It only allocates if the total amount of planes is greater than 3.

Yes, that seems to be a good idea. I will try to use that, to simplify also some of the other code.

rewritten the clipping algorithm using the proposed concept

It should work the same or maybe better than before. It is much simpler, but I am unsure if it is faster. But since it is soo much cleaner codewise I like this version more.

Henrik Dick (weasel) marked an inline comment as done.Jun 16 2020, 6:51 PM

fixed rotation plane constraint

When rotating around a specified axis, the vertices will now stay in the plane perpendicular to the rotation axis.

Germano Cavalcante (mano-wii) added inline comments.Jun 16 2020, 9:31 PM

source/blender/editors/transform/transform_convert.c
1425	At first I was confused about what this `special_axis` is, but now I see that it is used as the normal of the constraint plane. If that is the case, the code is wrong. The spacemtx axis will not always be orthogonal.
1434	Do not copy this entire matrix, refer to it as a pointer. MirrorClippingData *mcd = &mcd_arr[k];

made requested changes
added missing space conversion

Henrik Dick (weasel) marked 2 inline comments as done.Jun 16 2020, 11:41 PM

added missing check for tc-use_local_mat

rebase
cleanup

This patch is still up for review

Harbormaster completed remote builds in B14133: Diff 36348.Apr 20 2021, 10:51 PM

It doesn't look good that MirrorClippingData is created with every interaction.
It should be cached for each TransDataContainer of types that support mirror clipping (Mesh and Curve).

So the ideal here would be to create the MirrorClippingData only once during the creation of the TransData for mesh and for curve and pass this data as a parameter of the clipMirrorModifier function.

See the TransCustomDataLayer, you can rename and increment this struct to contain the MirrorClippingData.

cache clipping planes in custom

Not sure about the names of the functions and structs.

Harbormaster completed remote builds in B14205: Diff 36465.Apr 24 2021, 12:30 PM

The idea is getting better :)

I will do some cleanups in the master code to simplify this patch.

I have some remarks however:

source/blender/editors/transform/transform_convert.h
64 ↗	(On Diff #36465)	Not sure about having this struct? The "clip_planes_len" can be returned in a parameter.
75 ↗	(On Diff #36465)	Perhaps a better is `transform_convert_clip_planes_from_modifiers`? Since there is something being allocated in the result, in my opinion a better suffix would be `_create` instead of `_calc`. But in order not to get too verbose, it might be better to leave it without a suffix.
79 ↗	(On Diff #36465)	For new functions in this file, the prefix `transform_convert_` is more usual.
source/blender/editors/transform/transform_convert_curve.c
81 ↗	(On Diff #36465)	Pick: `tc_curve_clip_planes_free_fn`
194 ↗	(On Diff #36465)	Create only when used?
source/blender/editors/transform/transform_convert_mesh.c
801 ↗	(On Diff #36465)	While this saves memory, it doesn't seem worth it to add a new pointer for a different data. Could the members of `TransCustomDataLayer` and `TransClippingPlaneData` be part of the same `TransCustomDataContainer`?
808 ↗	(On Diff #36465)	`tc_mesh_customdata_free_fn`
1028 ↗	(On Diff #36465)	The TransCustomDataContainer could be allocated only when required (lazy initialization?). And perhaps its creation could be moved to a new function.
1086 ↗	(On Diff #36465)	`tc_mesh_container_customdata_free_fn`
1621 ↗	(On Diff #36465)	I don't see why keep these UNUSED parameters
1661 ↗	(On Diff #36465)	This comment looks better located just above `tc_mesh_apply_to_mirror`.

requested changes

Harbormaster completed remote builds in B14207: Diff 36473.Apr 24 2021, 9:43 PM

Thanks for the feedback, I have some questions though:

source/blender/editors/transform/transform_convert.h
64 ↗	(On Diff #36465)	Not sure either, but I created it to package the array and the length (see other comment), could also make sure that the array is one longer and leave the last element 0 to abort the iteration.
source/blender/editors/transform/transform_convert_mesh.c
801 ↗	(On Diff #36465)	It's not only saving memory. In fact the main goal of this struct was to avoid rewriting a lot of the code for `TransCustomDataLayer`, because it assumes that when `TransCustomDataLayer` is allocated, then it's also filled with at least a `BMesh`. Could do the rewrite though If you think that's better. Prior cleanup could also make this much easier.

Rebase master

I hope you don't mind, but I changed the patch accordingly.
(It's easier than explaining).

Feel free to edit from here.
I still have to study the solution further.

Harbormaster completed remote builds in B14208: Diff 36477.Apr 25 2021, 3:08 AM

@Germano Cavalcante (mano-wii) Good move. Explaining code is sometimes difficult.

Although I would like to have the changes which just move functions around
in a cleanup patch before (or after) this one, so review gets a little easier again.

Rebase on Master

Harbormaster completed remote builds in B14234: Diff 36522.Apr 26 2021, 3:16 PM

Rebase onto master

I have the impression that transform_convert_clip_mirror_modifier_apply could be simplified/optimized.
I think it needs a closer look.

Harbormaster completed remote builds in B21288: Diff 49789.Mar 28 2022, 8:19 PM

Revision Contents
Changeset List

Path

Size

source/

blender/

editors/

transform/

transform_convert.c

238 lines

Diff 26000

View Options

source/blender/editors/transform/transform_convert.c

Improve Mirror Modifier clipping behaviour with constraint translation axisNeeds ReviewPublicActions