Page MenuHome

Improve Mirror Modifier clipping behaviour with constraint translation axis
Needs ReviewPublic

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

Details

Reviewers
Germano Cavalcante (mano-wii)
Group Reviewers
Modeling
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

Henrik Dick (weasel) requested review of this revision.May 1 2020, 12:03 PM
Henrik Dick (weasel) created this revision.
Henrik Dick (weasel) edited the summary of this revision. (Show Details)
  • 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) 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).

  • 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.

@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:

1diff --git a/source/blender/editors/transform/transform_convert.c b/source/blender/editors/transform/transform_convert.c
2index 8496642185d..92d3eb974f2 100644
3--- a/source/blender/editors/transform/transform_convert.c
4+++ b/source/blender/editors/transform/transform_convert.c
5@@ -1342,43 +1342,54 @@ void clipMirrorModifier(TransInfo *t)
6 FOREACH_TRANS_DATA_CONTAINER (t, tc) {
7 Object *ob = tc->obedit;
8 ModifierData *md = ob->modifiers.first;
9- float tolerance[3] = {0.0f, 0.0f, 0.0f};
10- int axis = 0;
11+ bool is_constraint = (t->con.mode & CON_APPLY) != 0;
12
13 for (; md; md = md->next) {
14 if ((md->type == eModifierType_Mirror) && (md->mode & eModifierMode_Realtime)) {
15 MirrorModifierData *mmd = (MirrorModifierData *)md;
16
17 if (mmd->flag & MOD_MIR_CLIPPING) {
18- axis = 0;
19+ int axis = 0;
20 if (mmd->flag & MOD_MIR_AXIS_X) {
21 axis |= 1;
22- tolerance[0] = mmd->tolerance;
23 }
24 if (mmd->flag & MOD_MIR_AXIS_Y) {
25 axis |= 2;
26- tolerance[1] = mmd->tolerance;
27 }
28 if (mmd->flag & MOD_MIR_AXIS_Z) {
29 axis |= 4;
30- tolerance[2] = mmd->tolerance;
31 }
32- if (axis) {
33- float mtx[4][4], imtx[4][4];
34- int i;
35
36+ if (axis) {
37+ float mirror_plane[3][4];
38+ int mirror_plane_len = 0;
39+ float(*mirror_mtx)[4] = NULL;
40 if (mmd->mirror_ob) {
41- float obinv[4][4];
42+ mirror_mtx = mmd->mirror_ob->obmat;
43+ }
44+ else if (tc->use_local_mat) {
45+ mirror_mtx = tc->mat;
46+ }
47
48- invert_m4_m4(obinv, mmd->mirror_ob->obmat);
49- mul_m4_m4m4(mtx, obinv, ob->obmat);
50- invert_m4_m4(imtx, mtx);
51+ for (int i = 0; i < 3; i++) {
52+ if (axis & (1 << i)) {
53+ float *mirror_pl = mirror_plane[mirror_plane_len++];
54+ if (mirror_mtx) {
55+ plane_from_point_normal_v3(mirror_pl, mirror_mtx[3], mirror_mtx[i]);
56+ mirror_pl[3] /= normalize_v3(mirror_pl);
57+ }
58+ else {
59+ zero_v4(mirror_pl);
60+ mirror_pl[i] = 1.0f;
61+ }
62+ }
63 }
64
65+ float tolerance = mmd->tolerance;
66 TransData *td = tc->data;
67- for (i = 0; i < tc->data_len; i++, td++) {
68- int clip;
69+ for (int i = 0; i < tc->data_len; i++, td++) {
70 float loc[3], iloc[3];
71+ bool clip = false;
72
73 if (td->loc == NULL) {
74 break;
75@@ -1391,34 +1402,32 @@ void clipMirrorModifier(TransInfo *t)
76 copy_v3_v3(loc, td->loc);
77 copy_v3_v3(iloc, td->iloc);
78
79- if (mmd->mirror_ob) {
80- mul_m4_v3(mtx, loc);
81- mul_m4_v3(mtx, iloc);
82+ if (tc->use_local_mat) {
83+ mul_m4_v3(tc->mat, loc);
84+ mul_m4_v3(tc->mat, iloc);
85 }
86
87- clip = 0;
88- if (axis & 1) {
89- if (fabsf(iloc[0]) <= tolerance[0] || loc[0] * iloc[0] < 0.0f) {
90- loc[0] = 0.0f;
91- clip = 1;
92+ for (int j = 0; j < mirror_plane_len; j++) {
93+ float t_iloc = plane_point_side_v3(mirror_plane[j], iloc);
94+ float t_loc = plane_point_side_v3(mirror_plane[j], loc);
95+ if (fabs(t_iloc) <= tolerance || (t_loc * t_iloc) < 0.0f) {
96+ clip = true;
97+ if (is_constraint) {
98+ /* Intersect movement with the mirror plane. */
99+ float move_dist = (t_iloc - t_loc);
100+ if (move_dist) {
101+ interp_v3_v3v3(loc, iloc, loc, t_iloc / move_dist);
102+ continue;
103+ }
104+ }
105+ /* Project the location on the mirror plane. */
106+ madd_v3_v3fl(loc, mirror_plane[j], -t_loc);
107 }
108 }
109
110- if (axis & 2) {
111- if (fabsf(iloc[1]) <= tolerance[1] || loc[1] * iloc[1] < 0.0f) {
112- loc[1] = 0.0f;
113- clip = 1;
114- }
115- }
116- if (axis & 4) {
117- if (fabsf(iloc[2]) <= tolerance[2] || loc[2] * iloc[2] < 0.0f) {
118- loc[2] = 0.0f;
119- clip = 1;
120- }
121- }
122 if (clip) {
123- if (mmd->mirror_ob) {
124- mul_m4_v3(imtx, loc);
125+ if (tc->use_local_mat) {
126+ mul_m4_v3(tc->imat, loc);
127 }
128 copy_v3_v3(td->loc, loc);
129 }

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).

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):

Henrik Dick (weasel) added a comment.EditedMon, Jun 15, 2:59 PM

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

Henrik Dick (weasel) updated this revision to Diff 25913.EditedMon, Jun 15, 3:14 PM
  • 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?

Henrik Dick (weasel) added a comment.EditedMon, Jun 15, 4:16 PM

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...

Henrik Dick (weasel) added a comment.EditedMon, Jun 15, 4:48 PM

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
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.Mon, Jun 15, 7:03 PM
This revision now requires changes to proceed.Mon, Jun 15, 7:03 PM
Henrik Dick (weasel) updated this revision to Diff 25962.EditedTue, Jun 16, 10:16 AM
Henrik Dick (weasel) marked 5 inline comments as done.
  • 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.Tue, Jun 16, 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.

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.Tue, Jun 16, 11:41 PM
  • added missing check for tc-use_local_mat