Details

Reviewers

Group Reviewers

Maniphest Tasks

T82334: Mirror Global mirrors on local axis
T68521: Unexpected behaviour of object mode mirror, doesn't mirror rotation.

Commits

rBec39d8de4adf: Transform: Correct Mirror for Object Mode along arbitrary axis

Summary

Fixes T68521.
The Problem described there is that object are not properly mirrored in object mode. It was assumed that there is no way to mirror an object about an arbitrary axis. But clearly there must be a way, since no scew happens during the mirror operation and the mirrored state preserves all angles of the object.
For mirroring an object that has an arbitrary rotation along the X axis this is the procedure:

mirror x location.
negate x scale of the
negate y and z component of the rotation (independent of which rotation mode is used).

This patch applies this knowledge for all angles and axes to finally make the mirror operation work in object mode. Since the mirror operator now uses different code specifically for mirror, it is also faster than previous versions, but that is obviously neglegtible. The new mirror function has the downside, that it can not (in this form) be used with proportional editing. This is no problem since the old behavior can still be replicated by scaling with -1 along any axis. The solution to get perfect mirrors can not work for scaling in general, because in that case there could be scew created.

I didn't test every use-case, as I currently don't have the time, so testing of this patch is highly encouraged. I am almost certain there are some unhandled cases left in this initial state.

Diff Detail

Repository: rB Blender

Event Timeline

Henrik Dick (weasel) requested review of this revision.Nov 21 2020, 11:04 PM

Henrik Dick (weasel) created this revision.

Good to see this operator being worked on.
I tested it and so far I haven't found any problem :)
If "proportional editing" doesn't work anymore, it should be removed from the operator's properties, right?

diff --git a/source/blender/editors/transform/transform_ops.c b/source/blender/editors/transform/transform_ops.c
index c455dd55b8a..5153fdedcae 100644
--- a/source/blender/editors/transform/transform_ops.c
+++ b/source/blender/editors/transform/transform_ops.c
@@ -1040,8 +1040,7 @@ static void TRANSFORM_OT_mirror(struct wmOperatorType *ot)
   ot->poll = ED_operator_screenactive;
   ot->poll_property = transform_poll_property;
 
-  Transform_Properties(
-      ot, P_ORIENT_MATRIX | P_CONSTRAINT | P_PROPORTIONAL | P_GPENCIL_EDIT | P_CENTER);
+  Transform_Properties(ot, P_ORIENT_MATRIX | P_CONSTRAINT | P_GPENCIL_EDIT | P_CENTER);
 }
 
 static void TRANSFORM_OT_bbone_resize(struct wmOperatorType *ot)

Also, the ElementMirror function seems to be very specific to the mode.
I'm not sure if that function needs to go to transform_mode.c.
There are only functions that are shared between multiple modes.

remove proportional editing flag
fix scale lock not considered
move mirror code

I made some size functions public now, so they can be used by mirror.
I noticed that it still does not work properly in armatures, but I have
no idea how to fix that. It does not consider roll of the bones and I
dont see a trivial way to do so.

@Germano Cavalcante (mano-wii) Can I use orient_axis instead of my current hack for getting the axis? (didn't test it yet)

Harbormaster completed remote builds in B11400: Diff 31311.Nov 23 2020, 10:50 PM

In D9625#239232, @Henrik Dick (weasel) wrote:

@Germano Cavalcante (mano-wii) Can I use orient_axis instead of my current hack for getting the axis? (didn't test it yet)

How can we choose multiple axis in this case?

Reviewing the hack, it might not be a hack if it were more readable.
You could replace the 7 with (CON_AXIS0 | CON_AXIS1 | CON_AXIS2)
Couldn't you use functions like getConstraintSpaceDimension, bitscan_forward_i or count_bits_i to simplify?

One thing to keep in mind is that if the constraint is from a plane defined for example by the axes (CON_AXIS0 | CON_AXIS2), the normal of that plane will not necessarily be the (CON_AXIS1), the space_matrix may not be orthogonal or one of the axes can be zero.

I saw a bug when enabling the 3 mirror axis (x, y and z). The y-axis has been ignored.

This revision now requires changes to proceed.Nov 25 2020, 2:11 PM

fix using XYZ at the same time
fix mirror for editmode with scaled rotated object
use bithack instead of log2

I found a case where mirror can't work correctly.
It's when there is an object with a scaled rotated parent.
It's impossible to get the global mirror there as it would need scew (AFAICT).

@Germano Cavalcante (mano-wii) when exactly can spacemtx be non orthogonal (scewed)?

I didn't find fail cases where mirror could work with the new patch.
So again testing is very welcome.

Harbormaster completed remote builds in B11420: Diff 31371.Nov 25 2020, 9:59 PM

In D9625#239641, @Henrik Dick (weasel) wrote:

@Germano Cavalcante (mano-wii) when exactly can spacemtx be non orthogonal (scewed)?

Some examples I can think of:

rotated children with parent scaled in one direction
Gimbal orientation
Matrix explicitly passed to the python operator

Ok I though a bit more about the scewd spacemtx. For the general mirror operation I always (if possible) want to mirror the visual geometry in global space as that is the most intuitive and useful. Other types of mirrors can still be achieved by scaling.

If no mirror axis is selected a scewd spacemtx is fine.
If XYZ mirror axis (all of them) are selected a scewd spacemtx has an undefined result, so we need to make a decision. In the current patch my decision was to just invert all scale values of the objects and mirror them in global space. One other option would be to mirror on one axis after the other, but from testing I saw that that would be order dependent, so the current solution is much better.
If only X mirror axis is selected -> mirror on that axis. Simple. A scewd spacemtx doesn't matter as we are only using one column of it.
If XY mirror axis (or any permutation) is selected there are two possible solutions.
- mirror on Z and then on XYZ. A bit non intuitive for scewd spacemtx. Not dependent on the order of Z/XYZ mirror as the XYZ mirror in global space commutates with any other operation. Current solution.
- mirror on X then on Y. Dependent on the order of X/Y mirror operations if the matrix is scewd.

So the current approach to this operator seems to be the best I can think of in all cases. It does not work in all cases due to technical limitations, but it always tries to get as close as possible. I also didn't find any more non working fixable situations yet.

One last problem that I can see is that it does not flip the roll of mirrored bones.
I think that might be for another patch, but I also do not know how to do that.
(@Germano Cavalcante (mano-wii) clue? I really want it to be perfect)

Germano Cavalcante (mano-wii) added a task: T82334: Mirror Global mirrors on local axis.Nov 27 2020, 3:46 PM

In D9625#239876, @Henrik Dick (weasel) wrote:

(@Germano Cavalcante (mano-wii) clue? I really want it to be perfect)

See how transform_convert_aramature.c and other transform modes handle this value.
It would also be interesting to think of a way to unify the solution of rolls in different types of objects (such as curves).
Are the members *rotAngle and irotAngle of td->ext being used for bones and curves? Perhaps they can be used to increment the values of TransData.

@Germano Cavalcante (mano-wii) I looked at the file for roll handling, but it seems to be all over the place. It's is used from the field val but sometimes saved in td->extra. I don't understand it.

rotAngle is used for axis angle rotations currently (even though rotAxis is a float[4] so it should also be able to contain the angle).
So as far as I can tell it is unused for bones and curves, as there is no axis angle rotation there.
I think the structure could be changed to pack the angle of axis angle into the rotAxis and free rotAngle for curve tilt and bone roll.
I don't know if there is anyting with just a 2D rotation angle, but that should also go into rotAngle.

For this patch I think it has everything reasonable right now.
If bone rotation will be made possible via the rotate operator in the future, that would make the mirror also correctly mirror the roll I think.

Germano Cavalcante (mano-wii) added inline comments.Nov 30 2020, 3:04 PM

source/blender/editors/transform/transform_mode_mirror.c
45	It would be good to describe these parameters: /** * . * * \param axis * \param flip */ It is not clear what happens when the axis is less than zero or how this operator works when multiple axis are selected.
163	I had a little difficulty understanding this description. That way it would be easier to understand in my opinion: `Assuming that CON_AXIS0 < CON_AXIS1 < CON_AXIS2 and CON_AXIS2 is CON_AXIS0 << 2` But perhaps to avoid future errors, a BLI_assert can be applied here: `BLI_assert(CON_AXIS2 == (CON_AXIS0 << 2))`?
168	This operation is not clear. Would this be better? special_axis_bitmap = (axis_bitmap << 2) \| (axis_bitmap >> 1);
182	I'm finding this part of the code very confusing which impairs the quality of it. It can certainly be simplified and readable. For example: int bitmap_len = count_bits_i(axis_bitmap); int special_axis_bitmap = 0; if (!ELEM(bitmap_len, 0, 3)) { special_axis_bitmap = (bitmap_len == 2) ? ~axis_bitmap : axis_bitmap; special_axis = bitscan_forward_i(special_axis_bitmap); } ElementMirror(t, tc, td, special_axis, bitmap_len >= 2);

add comment for axis and flip
use branched logic with count_bits_i for readability

Thanks for the feedback.

I did the previous version of the axis logic because it's faster to don't have branches and
count_bits_i isn't as fast as a few left shifts, but I am fine with this more readable
solution as well as this part of the code is not performance critical.
I also added the suggested assert.

Harbormaster completed remote builds in B11459: Diff 31453.Nov 30 2020, 9:30 PM

Henrik Dick (weasel) marked 4 inline comments as done.Nov 30 2020, 9:31 PM

Germano Cavalcante (mano-wii) accepted this revision.Nov 30 2020, 10:21 PM

This revision is now accepted and ready to land.Nov 30 2020, 10:21 PM

Closed by commit rBec39d8de4adf: Transform: Correct Mirror for Object Mode along arbitrary axis (authored by Henrik Dick (weasel), committed by Germano Cavalcante (mano-wii)). · Explain WhyDec 2 2020, 1:29 PM

This revision was automatically updated to reflect the committed changes.

Germano Cavalcante (mano-wii) added a commit: rBec39d8de4adf: Transform: Correct Mirror for Object Mode along arbitrary axis.

Raimund Klink (Raimund58) awarded a token.Dec 2 2020, 8:19 PM

Roman (roman13) awarded a token.Dec 7 2020, 7:17 PM

Roman (roman13) rescinded a token.

Roman (roman13) awarded a token.