Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link accumulated fixes #2491

Closed
wants to merge 13 commits into from
Closed

Conversation

realthunder
Copy link
Collaborator

Vector3d v(0,0,1);
return std::fabs(multVec(v).GetAngle(q.multVec(v))) < tol;
double dot = q.quat[0]*quat[0]+q.quat[1]*quat[1]+q.quat[2]*quat[2]+q.quat[3]*quat[3];
return fabs(dot) >= 1.0 - tol;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a problem, I think. fabs(dot) is very insensitive to changes to any of the quaternions, because the value is at its peak (i.e. the derivative is zero). That means, the actual tolerance is more like sqrt(tol), so if you ask for tolerance of 1e-12, the comparison is sensitive to something like 1e-6 radians.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To consider: return length(Q1-Q2) < tol || length(Q1+Q2) < tol

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically the values Q1,Q2,Q3 of a quaternion keep the information about the rotation axis and Q4 keeps the information about the angle.
With the new implementation the tol has no geometric meaning any more but a pure algebraic meaning and I think it's hard to see how it behaves for different quaternions.
If there is a slight difference of the values Q1-Q3 of two quaternions then it presumably has another effect as when Q4 is slightly different.

So, the old implementation has a clear geometric meaning that can be easily understood but is just not correctly implemented.

A good to understand implementation could be:
multiply the one quaternion with the inverse of the other quaternion. If both are similar we should get a quaternion that is almost the identity quaternion.
Now multiply the vector (1,0,0) with the quaternion and calculate the angle between the output and (1,0,0). If the angle is <= tol the quaternions can be considered similar.

Copy link
Contributor

@DeepSOIC DeepSOIC Sep 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically the values Q1,Q2,Q3 of a quaternion keep the information about the rotation axis and Q4 keeps the information about the angle.

Not even close.

>>> App.Rotation(App.Vector(1,1,1),1e-6/6.28*360).Q
(2.888215548143538e-07, 2.888215548143538e-07, 2.888215548143538e-07, 0.9999999999998749)

EDIT: well, close. But Q1,Q2,Q3 are more like axis multiplied by angle.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I said basically, not exactly!
When a normalized axis (x,y,z) and an angle (rad) is given then:
Q1 = x * sin(rad/2)
Q2 = y * sin(rad/2)
Q3 = z * sin(rad/2)
Q4 = cos(rad/2)

sin(rad/2) is applied to all coordinates of the axis so (Q1,Q2,Q3) has the exact same direction (orientation of the vector can be flipped) as the axis.

And when you look at your example you will see that the first three values of the quaternion are equal as of the axis (1,1,1).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, got it.

I am still under impression that quaternions to 3d rotations are pretty much the same as representing 2d rotations with a unit-length vector x,y = cos, sin. There, you can quickly figure out the amount the two rotations are different by just measuring length of difference of the two vectors representing them (for small angles, the length of vector difference is about equal to the angle difference).

So I used the same trick with quaternions, adding that two equal and opposite quaternions are the same.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it further the trick with the inverted quaternion doesn't work either:
r1=App.Rotation(App.Vector(0,1,0),20)
r2=App.Rotation(App.Vector(1,0,0),15)
r3=r2.multiply(r1)

r4=r1.multiply(r3.inverted())
r4.multVec(App.Vector(1,0,0)) # -> Vector (1.0, 6.87953070464688e-18, -9.05707399495939e-19)
r4 # Rotation (-0.13052619222005157, 0.0, 3.469446951953614e-18, 0.9914448613738104)

So, this brings me back to my original idea to check the input and output of two vectors.

Copy link
Contributor

@wwmayer wwmayer Sep 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I detected that Coin's SbRotation class offers a equal() method with a tolerance and the implementation is based on SbVec4f that looks like this:

float xdist = this->vec[0] - v[0];
float ydist = this->vec[1] - v[1];
float zdist = this->vec[2] - v[2];
float wdist = this->vec[3] - v[3];

if((xdistxdist + ydistydist + zdistzdist + wdistwdist) <= tolerance)
return TRUE;
return FALSE;

So, then it's probably best to use the same implementation to avoid inconsistencies between SbRotation and Rotation.

Remark:
Coin's implementation can be optimized quite a bit. Assuming we have two quaternions Q=(q1,q2,q3,q4) and R=(r1,r2,r3,r4) then Coin checks this:
(q1-r1)^2 + (q2-r2)^2 + (q3-r3)^2 + (q4-r4)^2 <= tol
<=>
(q1^2 - 2q1r1 + r1^2) + (q2^2 - 2q2r2 + r2^2) + (q3^2 - 2q3r3 + r3^2)+(q4^2 - 2q4r4 + r4^2) <= tol
<=>
(q1^2+q2^2+q3^2+q4^2) + (r1^2+r2^2+r3^2+r4^2) - 2*(q1r1 + q2r2 + q3r3 + q4r4) <= tol
<=> (the sum of squares of a quaternion is 1)
2 - 2*(q1r1 + q2r2 + q3r3 + q4r4) <= tol
<=>
q1r1 + q2r2 + q3r3 + q4r4 >= 1-tol/2

So, then we are back to realthunder's implementation with the difference that we must check with tol/2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some precision tests, let's continue there:
https://forum.freecadweb.org/viewtopic.php?f=10&t=39231

@wwmayer
Copy link
Contributor

wwmayer commented Sep 10, 2019

Merged.

I skipped the changes for the Mesh module because this is not the way it should behave -- I will work on another solution.
I have adjusted and extended the change for the Rotation and kept the method name isSame().

@wwmayer wwmayer closed this Sep 10, 2019
@realthunder
Copy link
Collaborator Author

I skipped the changes for the Mesh module because this is not the way it should behave -- I will work on another solution.

Can you please check the implementation here?

Linked mesh will still get full selection highlight, because it is handled in upper group node. This is actually an optimization of render caching and selection clear for deep hierarchy.

mesh-select

@wwmayer
Copy link
Contributor

wwmayer commented Sep 17, 2019

Linked mesh will still get full selection highlight, because it is handled in upper group node. This is actually an optimization of render caching and selection clear for deep hierarchy.

This automatic highlighting is a real problem for huge meshes (e.g. 13 million triangles) because it takes a couple of seconds to do the rendering after selecting it in the tree and freezes the GUI.

That's why highlighting of meshes was always done with a bounding box which doesn't cause any extra cost in terms of rendering.

@realthunder
Copy link
Collaborator Author

That's why highlighting of meshes was always done with a bounding box which doesn't cause any extra cost in terms of rendering.

I did some work to improve render caching which is already in upstream. Maybe you can do some test on your side to confirm. My simple test shows that full selection highlight is not that slow comparing to bounding box, because the selection rendering will be cached. When you load a big mesh, initial selection takes some time, but later selection is faster. It will consume more graphics memory, though.

You can try this file. The mesh inside contains 1M faces. The screencast below shows the effect of highlight caching. Treeview mouse over highlight performs rendering equivalent to full selection highlight in order to pop up obscured objects.

mesh-highlight

@wwmayer
Copy link
Contributor

wwmayer commented Sep 19, 2019

OK, I will have a look.

@realthunder
Copy link
Collaborator Author

OK, I will have a look.

I have refactor the box selection style in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants