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
Closed
Link accumulated fixes #2491
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
f0464d2
Gui: fix active object highlight
realthunder 8b063c6
Mesh: fix selection
realthunder bb9bf3b
Gui: restore tree view content menu action order
realthunder f67b33d
Gui: fix property view flickering
realthunder 3f87755
Gui: fix property view on multiple sub-element selections
realthunder aab1d74
Base: fix verbose checking in Console::Log()
realthunder 9e93f92
Gui: add Gui.updateCommands() for manual command status update
realthunder 494d87f
Gui: minor adjustment on command status update
realthunder 9e9aed1
Base: fix Matrix4D::hasScale() tol checking
realthunder d9f452e
Base: rename Rotation::isSame() to isSimilar(), fix implementation
realthunder 0e789a7
App: improve document::readObjects()
realthunder fa31b94
Gui: improve code readability
realthunder 60a7187
Split App::AutoTransaction into its own file
realthunder File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not even close.
EDIT: well, close. But Q1,Q2,Q3 are more like axis multiplied by angle.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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