Regression: some dimensions cause "setValue(matrix): Could not determine the rotation."

[hko]

Recent commit 6c3efbdb3bd339d112e53801d4ff2ea840c8cfc8 made changes to Rotation.cpp that includes throwing an error with message

Code: Select all

setValue(matrix): Could not determine the rotation.

I have a FreeCAD file that, when loaded with a build that includes that commit, triggers that error 16 times. I minimized the file down to two draft dimensions (the full file has tens of dimensions so not all dimensions are problematic), each triggering the error 8 times. The minimized file is attached.

Note that the dimensions are grayed in the tree view and not visible in the 3D view. They should be visible (they are if I load the original file into a build before the 6c3efbdb3bd339d112e53801d4ff2ea840c8cfc8 commit). If I try to make Dimension034 visible, the dimension does not become visible and I get

Code: Select all

<Exception> Rotation.cpp(226): void Base::Rotation::setValue(const Base::Matrix4D&) -- setValue(matrix): Could not determine the rotation.

That exception does not happen when trying to make Dimension081 visible, but it doesn't become visible either.

Interestingly, if I load the minimized file into a build from commit bdb6e1bd552505c18795d665410a5527ea00fc49 (just before the commit 6c3efbdb3bd339d112e53801d4ff2ea840c8cfc8), both dimensions are also hidden. (They are not hidden if I load the non-minimized full file that I have not touched with the broken build containing 6c3efbdb3bd339d112e53801d4ff2ea840c8cfc8.) If I try to toggle them visible, they become visible in the 3D view but they remain gray in the tree view. If I create a new dimension, the two existing dimensions then become non-grayed in the tree view. If I toggle their (the two original ones) visibility again, they become hidden in the 3D view but remain non-grayed in the tree view. If I then toggle the visibility of the new dimension, all three become grayed in the tree view.

Finally, when I was minimizing the file, I deleted a few dimensions, saved the file, loaded again to see if something changed, deleted a few more dimensions, etc. While doing that, I got

Code: Select all

Traceback (most recent call last):
  File "/usr/local/freecad-latest/Mod/Draft/draftviewproviders/view_dimension.py", line 693, in onChanged
    self.updateData(obj, "Start")
  File "/usr/local/freecad-latest/Mod/Draft/draftviewproviders/view_dimension.py", line 402, in updateData
    and not DraftVecUtils.isNull(obj.Direction)):
  File "/usr/local/freecad-latest/Mod/Draft/DraftVecUtils.py", line 550, in isNull
    p = precision()
  File "/usr/local/freecad-latest/Mod/Draft/DraftVecUtils.py", line 62, in precision
    return params.GetInt("precision", 6)
<class 'RecursionError'>: maximum recursion depth exceeded while calling a Python object

So far I haven't been able to reproduce that but I haven't had time to try very hard.

test4.FCStd

(4.59 KiB) Downloaded 27 times

Code: Select all

OS: Ubuntu 20.04.5 LTS (KDE/plasma)
Word size of FreeCAD: 64-bit
Version: recent master, see hash
Build type: Debug
Branch: master
Hash: 4adf98e369417e5c2cb00eef3eef8a76daa30011
Python 3.8.10, Qt 5.12.8, Coin 4.0.0, Vtk 7.1.1, OCC 7.3.0
Locale: English/United States (en_US)
Installed mods: 
  * 3DfindIT 1.2.0
  * Render 2023.2.0
  * BIM 2021.12.0
  * Reinforcement
  * Manipulator 1.5.0
  * Assembly4.backup1662941442.2770898
  * parts_library
  * Assembly4 0.12.5
  * fasteners 0.4.54
  * Help 1.0.3
  * Silk 1.0.0
  * dodo 1.0.0
  * Curves 0.6.7

[chennes]

@Jolbas

[Jolbas]

@chennes I need some time to investigate. I could temporarily add a fallback to previous behaviour instead of throwing an exception when there is a Base::ScaleType::Other type of matrix. Or is it better to revert the entire commit until it's reworked and safe?

[chennes]

Your call: I don't really like reverting entire PRs if there is another option, so I'd say if you have time my preference is for the fallback option.

[wmayer]

Looking at what happens there is a case that the 3x3 sub-matrix almost describes a rotation but it exceeds the default tolerance of 1e-9
If you compare the first and third row you will see that the lengths are < 1 and > 1.

Code: Select all

(0, -0.9999934297791958, 0, 0)
(0.9999934297791958, 0, 0, 0)
(0.003624968750279073, 0, 1, 0)
(0, 0, 0, 1)

Also with the old implementation of Matrix4D::hasScale() it said it's not a rotation matrix. The problem is that in the past the matrix was silently accepted and now it raises an exception.

When increasing the tolerance to 1e-5 then you will see the difference of input and output matrix:

Code: Select all

m=App.Matrix(0, -0.9999934297791958, 0, 0, 0.9999934297791958, 0, 0, 0, 0.003624968750279073, 0, 1, 0, 0, 0, 0, 1)
p=App.Placement(m)
m
p.toMatrix()

Matrix ((0,-0.999993,0,0),(0.999993,0,0,0),(0.00362497,0,1,0),(0,0,0,1))
Matrix ((-4.9276e-06,-0.999998,-0.00181247,0),(0.999998,-1.64254e-06,-0.00181248,0),(0.00181247,-0.00181248,0.999997,0),(0,0,0,1))

[Jolbas]

I had a look at this now. Maybe we could make the tolerance a bit higher for Rotation(matrix) to resolve more matrices but 1e-5 seems a bit too high. I would say not more than 1e-7 and in that case add a this->normalize(); at the end.

I think we could blame DraftVecUtils.getPlaneRotation(u,v,w) here that does not return a pure rotation matrix unless u, v and w is perfectly perpendicular normalized vectors. The name suggests it should. Or is it expected to return a skewed matrix? I would ignore the w vector and

Code: Select all

    u = Vector(u)
    u.normalize()
    w = u.cross(v)
    if not w.Length:
        return None
    w.normalize()
    v = w.cross(u)

Edit: added check for parallell u and v

[user1234]

Stupid question from a stupid user, have it to do with,

when you edit a placement in Euler angles and get in the properties axis (angle, axis x, axis y, axis y) back?


Greetings
user1234

[Jolbas]

when you edit a placement in Euler angles and get in the properties axis (angle, axis x, axis y, axis y) back?

Setting the rotation by Euler angles doesn't use a matrix to calculate angle and axis so it shouldn't be able to throw this exception.

[user1234]

Setting the rotation by Euler angles doesn't use a matrix to calculate angle and axis so it shouldn't be able to throw this exception.

Thanks. Just asking because i noticed, that when i place something with the placement dialog in Euler angles, the placement property get the angle/axis value. But it is then that unprecise, that when you edit with the dialog (just saying, Euler angles with expressions, to fix it) again, the value changes then back and forth. And then for TechDraw and some boolean operation, it is too unprecise and not usable more.

Greetings
user1234

[Jolbas]

Created an issue and a possible fix