Sketcher: Fix Issue 6283 #9328
Sketcher: Fix Issue 6283 #9328
Conversation
=================================================== This commit exposes the scale factor used.
========================================= The scaling factor used was the one of the ViewProviderSketch, however ZoomTranslation uses its own scaling factor. fixes FreeCAD#6283
| } | ||
|
|
||
| SoZoomTranslation::SoZoomTranslation() | ||
| SoZoomTranslation::SoZoomTranslation(): scaleFactor(0) |
There was a problem hiding this comment.
Default 0 for scaleFactor might not be the best idea, especially if there is division involved. 1 sounds better, depending on what the definition is.
There was a problem hiding this comment.
The objective is to store whatever the ZoomTranslation calculates.
Retrieving the scaleFactor before (internal) calculation will always result in a wrong value. This means that 1 will be as wrong as 0.
There is no risk of exception as the the scale factor is multiplied to a relative translation.
Then, perhaps 0 best serves a user of the client code to realise the mistake, if trying to retrieve a scale before the node is actually in running in the scenegraph.
| @@ -96,7 +96,7 @@ void SoZoomTranslation::doAction(SoAction * action) | |||
| SbVec3f absVtr = this->abPos.getValue(); | |||
| SbVec3f relVtr = this->translation.getValue(); | |||
|
|
|||
| float sf = this->getScaleFactor(action); | |||
| float sf = this->calculateScaleFactor(action); | |||
| // For Sketcher Keep Z value the same | |||
| relVtr[0] = (relVtr[0] != 0) ? sf * relVtr[0] : 0; | |||
There was a problem hiding this comment.
This is the same as relVtr[0] *= sf;
There was a problem hiding this comment.
Yes, you are indeed right. But I did not write that line...
I have also seen other things I do not like from this class (such as a hardcoded 5 in calculateScaleFactor).
I have fought the temptation to make a full review of SoZoomTranslation internal workings at this time.
Just adding a way to retrieve the scaling factor it uses, because that is what is necessary to fix the actual bug of the issue.
If you have time to do a full review and wish to do it, feel free to make a PR. It is welcome.
There was a problem hiding this comment.
Maybe eventually. We could create an issue about it, but then again we'd need as many as the number of files qe have!
| @@ -116,7 +116,7 @@ void SoZoomTranslation::getMatrix(SoGetMatrixAction * action) | |||
| SbVec3f absVtr = this->abPos.getValue(); | |||
| SbVec3f relVtr = this->translation.getValue(); | |||
|
|
|||
| float sf = this->getScaleFactor(action); | |||
| float sf = this->calculateScaleFactor(action); | |||
| // For Sketcher Keep Z value the same | |||
| relVtr[0] = (relVtr[0] != 0) ? sf * relVtr[0] : 0; | |||
There was a problem hiding this comment.
See previous comment.
No description provided.