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

Fix PartDesign::Mirrored #816

Closed
wants to merge 7 commits into from

Conversation

kkremitzki
Copy link
Member

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, please confirm the following:

  • Branch rebased on latest master git pull --rebase upstream master
  • Unit tests confirmed to pass by running ./bin/FreeCAD --run-test 0
  • Commit message is well-written
  • Commit message includes issue #<id> or fixes #<id> where <id> is the associated MantisBT issue id if one exists

And please remember to update the Wiki with the features added or changed once this PR once it is merged.


Commits fix PartDesign::Mirrored behavior and add two unit tests for it. More detailed explanation available at https://kkremitzki.github.io/blog/gsoc-week-1-recap/

mtrx[0][0] = scale * m(1,1);
mtrx[0][1] = scale * m(1,2);
mtrx[0][2] = scale * m(1,3);
mtrx[0][0] = m(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.

Removing the scale factor here makes this function inconsistent for OCC < 7.0 and >= 7.0
So, what is the reason of removing it?

Copy link
Member Author

Choose a reason for hiding this comment

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

gp_Trsf::VectorialPart returns the 3x3 matrix including the scale factor now [1], an equivalent change to me removing the (redundant) scale factor multiplication here would be to change to gp_Trsf::HVectorialPart [2] which does not include the scale factor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok. Thx for the pointer.

self.Doc.recompute()
self.failUnless(self.Mirrored.Shape.Volume < 2.0)
self.failUnless(self.Mirrored.Shape.Volume == 1.9999999999999993)
Copy link
Contributor

Choose a reason for hiding this comment

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

Exact comparison with floating numbers is a bad thing because it's simply too fragile. On my local system this unit test fails now. Also note that functions like failUnless are deprecated and should be replaced with their assert* equivalents.
In this case you should use assertAlmostEqual as described here https://docs.python.org/2/library/unittest.html

self.Mirrored.MirrorPlane = (self.Doc.XY_Plane, [""])
self.Body.addObject(self.Mirrored)
self.Doc.recompute()
self.failUnless(self.Mirrored.Shape.Volume == 1.9999999999999993)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same problem as above.

@wwmayer
Copy link
Contributor

wwmayer commented Jun 15, 2017

Merged + correction added

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