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

[Plot] Remove the workbench, leave the module #4971

Merged
merged 5 commits into from Aug 23, 2021

Conversation

sanguinariojoe
Copy link
Contributor

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, we ask you to conform to the following items. Pull requests which don't satisfy all the items below might be rejected. If you are in doubt with any of the items below, don't hesitate to ask for help in the FreeCAD forum!

  • Your pull request is confined strictly to a single module. That is, all the files changed by your pull request are either in App, Base, Gui or one of the Mod subfolders. If you need to make changes in several locations, make several pull requests and wait for the first one to be merged before submitting the next ones
  • In case your pull request does more than just fixing small bugs, make sure you discussed your ideas with other developers on the FreeCAD forum
  • Your branch is rebased on latest master git pull --rebase upstream master
  • All FreeCAD unit tests are confirmed to pass by running ./bin/FreeCAD --run-test 0
  • All commit messages are well-written ex: Fixes typo in Draft Move command text
  • Your pull request is well written and has a good description, and its title starts with the module name, ex: Draft: Fixed typos
  • Commit messages include issue #<id> or fixes #<id> where <id> is the FreeCAD bug tracker issue number in case a particular commit solves or is related to an existing issue on the tracker. Ex: Draft: fix typos - fixes #0004805

And please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 0.20 Changelog Forum Thread.


Following the plan discussed in #4890 (comment), I am removing the plot workbench shipped within the core (which was utterly broken and disabled), letting just the module (from FreeCAD.Plot import Plot)

The module is quite simple, requiring very little maintenance. Besides matplotlib is automatically providing handful tools to make some basic operations with the plot (Zoom, Save, ...). For more complex operations we still offer the Plot addon (https://github.com/FreeCAD/freecad.plot)

3 tests failed, but again they seem to be unrelated with this PR:

======================================================================
FAIL: test_ccx_cantilever_prescribeddisplacement (femtest.app.test_solver_calculix.TestSolverCalculix)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pepe/freecad/build/Mod/Fem/femtest/app/test_solver_calculix.py", line 208, in test_ccx_cantilever_prescribeddisplacement
    self.input_file_writing_test(get_namefromdef("test_"))
  File "/home/pepe/freecad/build/Mod/Fem/femtest/app/test_solver_calculix.py", line 397, in input_file_writing_test
    self.assertFalse(
AssertionError: 'Comparing /home/pepe/freecad/build/Mod/Fem/femtest/data/calculix/ccx_cantilever_prescribeddisplacement.inp to /tmp/FEM_unittests/solver_calculix_ccx_cantilever_prescribeddisplacement_53f0156aa1ca/Mesh.inp failed!\n--- \n+++ \n@@ -410 +410 @@\n-** reaction forces for Constraint displacement constraining translation\n+** reaction forces for Constraint displacement constraining translations\n' is not false : CalculiX write_inp_file for ccx_cantilever_prescribeddisplacement test failed.
Comparing /home/pepe/freecad/build/Mod/Fem/femtest/data/calculix/ccx_cantilever_prescribeddisplacement.inp to /tmp/FEM_unittests/solver_calculix_ccx_cantilever_prescribeddisplacement_53f0156aa1ca/Mesh.inp failed!
--- 
+++ 
@@ -410 +410 @@
-** reaction forces for Constraint displacement constraining translation
+** reaction forces for Constraint displacement constraining translations


======================================================================
FAIL: test_constraint_transform_beam_hinged (femtest.app.test_solver_calculix.TestSolverCalculix)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pepe/freecad/build/Mod/Fem/femtest/app/test_solver_calculix.py", line 270, in test_constraint_transform_beam_hinged
    self.input_file_writing_test(get_namefromdef("test_"))
  File "/home/pepe/freecad/build/Mod/Fem/femtest/app/test_solver_calculix.py", line 397, in input_file_writing_test
    self.assertFalse(
AssertionError: 'Comparing /home/pepe/freecad/build/Mod/Fem/femtest/data/calculix/constraint_transform_beam_hinged.inp to /tmp/FEM_unittests/solver_calculix_constraint_transform_beam_hinged_2f74d94be950/Mesh.inp failed!\n--- \n+++ \n@@ -3782,0 +3783,5 @@\n+** outputs --> dat file\n+** reaction forces for Constraint displacement constraining translations\n+*NODE PRINT, NSET=FemConstraintDisplacment, TOTALS=ONLY\n+RF\n+\n' is not false : CalculiX write_inp_file for constraint_transform_beam_hinged test failed.
Comparing /home/pepe/freecad/build/Mod/Fem/femtest/data/calculix/constraint_transform_beam_hinged.inp to /tmp/FEM_unittests/solver_calculix_constraint_transform_beam_hinged_2f74d94be950/Mesh.inp failed!
--- 
+++ 
@@ -3782,0 +3783,5 @@
+** outputs --> dat file
+** reaction forces for Constraint displacement constraining translations
+*NODE PRINT, NSET=FemConstraintDisplacment, TOTALS=ONLY
+RF
+


======================================================================
FAIL: test_frequency_beamsimple (femtest.app.test_solver_calculix.TestSolverCalculix)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/pepe/freecad/build/Mod/Fem/femtest/app/test_solver_calculix.py", line 286, in test_frequency_beamsimple
    self.input_file_writing_test(get_namefromdef("test_"))
  File "/home/pepe/freecad/build/Mod/Fem/femtest/app/test_solver_calculix.py", line 397, in input_file_writing_test
    self.assertFalse(
AssertionError: 'Comparing /home/pepe/freecad/build/Mod/Fem/femtest/data/calculix/frequency_beamsimple.inp to /tmp/FEM_unittests/solver_calculix_frequency_beamsimple_e703765b20a3/Mesh.inp failed!\n--- \n+++ \n@@ -17066 +17066 @@\n-** reaction forces for Constraint displacement constraining translation\n+** reaction forces for Constraint displacement constraining translations\n' is not false : CalculiX write_inp_file for frequency_beamsimple test failed.
Comparing /home/pepe/freecad/build/Mod/Fem/femtest/data/calculix/frequency_beamsimple.inp to /tmp/FEM_unittests/solver_calculix_frequency_beamsimple_e703765b20a3/Mesh.inp failed!
--- 
+++ 
@@ -17066 +17066 @@
-** reaction forces for Constraint displacement constraining translation
+** reaction forces for Constraint displacement constraining translations


----------------------------------------------------------------------

Copy link
Member

@chennes chennes left a comment

Choose a reason for hiding this comment

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

This compiles, runs, and the unit tests pass (except the three FEM tests that have been failing for a while). Tested on Windows 10:

OS: Windows 10 Version 2009
Word size of FreeCAD: 64-bit
Version: 0.20.25505 +3 (Git)
Build type: Release
Branch: pr/4971
Hash: b2910f1
Python version: 3.8.6+
Qt version: 5.15.1
Coin version: 4.0.1
OCC version: 7.5.0
Locale: English/United States (en_US)

@luzpaz luzpaz added the Missing: confirmation Missing confirmation from other testers label Aug 18, 2021
@luzpaz luzpaz mentioned this pull request Aug 18, 2021
@yorikvanhavre
Copy link
Member

Will that strategy not be confusing? There is now a minimal Plot module still in the FreeCAD core source code, and a more complete one available as an addon... Maybe users will be confused? How will they know they need to install the addon?
Wouldn't be better to either rename the "old" Plot or, for example, since it is only one file, why not integrate it to the ShaftWizard? So it becomes independent of Plot?

@sanguinariojoe
Copy link
Contributor Author

sanguinariojoe commented Aug 18, 2021

Will that strategy not be confusing? There is now a minimal Plot module still in the FreeCAD core source code, and a more complete one available as an addon... Maybe users will be confused? How will they know they need to install the addon?

They don't need to install the addon, that's the whole point. Herein we only have the module, i.e. the core component which allows every other module/workbench make plots, which already have the minimum matplotlib tools (see https://user-images.githubusercontent.com/1828501/123444869-a81ba100-d5d7-11eb-9bf3-8169108bf484.png).

The external addon is just providing a workbench to play with the plot styling. It is also offering a little bit more complete saving tool. For backward compatibility the addon is still deploying the Plot module, but it will be removed in the future (see FreeCAD/freecad.plot#16)

Wouldn't be better to either rename the "old" Plot or, for example, since it is only one file, why not integrate it to the ShaftWizard? So it becomes independent of Plot?

I consider this acceptable. However, I still prefer importing Plot with

from FreeCAD.Plot import Plot

rather than

from FreeCAD.PartDesign.WizardShaft import Plot

It is up to you guys


P.S. It should be highlighted that there is not any names collision between the module provided here (accessed as FreeCAD.Plot) and the workbench provided in the addon (accessed as freecad.plot). I suppose that's why @luzpaz suggested the new folders structure for addons

@luzpaz
Copy link
Contributor

luzpaz commented Aug 18, 2021

Do the CFDoF folks know about this proposed change?
CC @oliveroxtoby

@oliveroxtoby
Copy link
Contributor

Do the CFDoF folks know about this proposed change?
CC @oliveroxtoby

Thanks for the heads up! Good to know.

They don't need to install the addon, that's the whole point. Herein we only have the module, i.e. the core component which allows every other module/workbench make plots, which already have the minimum matplotlib tools (see https://user-images.githubusercontent.com/1828501/123444869-a81ba100-d5d7-11eb-9bf3-8169108bf484.png).

This would be great from the CfdOF point of view :)

@oliveroxtoby
Copy link
Contributor

I was testing this with CfdOF and just had one issue - it was a recurrence of a previous issue I had on my system that was fixed in the plot workbench. I have submitted a pull request on top of this branch to apply the same fix here (sanguinariojoe#1). Other than that it is working great for me.
image

Prevent matplotlib selecting the PyQt API instead of PySide2
@sanguinariojoe
Copy link
Contributor Author

Merged!

@yorikvanhavre
Copy link
Member

Okay then, indeed normal users will simply not see anything about Plot and install the WB if needed. But developers might still be confused. But OK, let's merge this and see how it goes. But let's keep an eye for confusion in people's minds, and think of a strategy if needed

@yorikvanhavre yorikvanhavre merged commit 103262d into FreeCAD:master Aug 23, 2021
@luzpaz luzpaz removed the Missing: confirmation Missing confirmation from other testers label Sep 2, 2021
@ceanwang
Copy link
Contributor

ceanwang commented Jun 1, 2022

I consider this acceptable. However, I still prefer importing Plot with

from FreeCAD.Plot import Plot

rather than

from FreeCAD.PartDesign.WizardShaft import Plot

Where is the Plot code located? inside PartDesign/WizardShaft?

waebbl added a commit to waebbl/gentoo that referenced this pull request Jul 25, 2022
Upstream has dropped the ship and plot workbenches recently. The
patch reflects these changes and drops the USE flags for it.
Note, that the plot module is still available, only the workbench
for separate working with plots has been removed.

See also FreeCAD/FreeCAD#4971 and
FreeCAD/FreeCAD#4900

Bug: https://bugs.gentoo.org/858308#c2
Signed-off-by: Bernd Waibel <waebbl-gentoo@posteo.net>
waebbl added a commit to waebbl/gentoo that referenced this pull request Jul 25, 2022
Upstream has dropped the ship and plot workbenches recently. The
patch reflects these changes and drops the USE flags for it.
Note, that the plot module is still available, only the workbench
for separate working with plots has been removed.

See also FreeCAD/FreeCAD#4971 and
FreeCAD/FreeCAD#4900

Also fixes some typos in pkg_postinst and minor code cleanup.

Bug: https://bugs.gentoo.org/858308#c2
Signed-off-by: Bernd Waibel <waebbl-gentoo@posteo.net>
waebbl added a commit to waebbl/gentoo that referenced this pull request Jul 28, 2022
Upstream has dropped the ship and plot workbenches recently. The
patch reflects these changes and drops the USE flags for it.
Note, that the plot module is still available, only the workbench
for separate working with plots has been removed.

See also FreeCAD/FreeCAD#4971 and
FreeCAD/FreeCAD#4900

Bug: https://bugs.gentoo.org/858308#c2
Signed-off-by: Bernd Waibel <waebbl-gentoo@posteo.net>
waebbl added a commit to waebbl/gentoo that referenced this pull request Jul 28, 2022
Upstream has dropped the ship and plot workbenches recently. The
patch reflects these changes and drops the USE flags for it.
Note, that the plot module is still available, only the workbench
for separate working with plots has been removed.

See also FreeCAD/FreeCAD#4971 and
FreeCAD/FreeCAD#4900

Also fixes some typos in pkg_postinst and minor code cleanup.

Bug: https://bugs.gentoo.org/858308#c2
Signed-off-by: Bernd Waibel <waebbl-gentoo@posteo.net>
waebbl added a commit to waebbl/gentoo that referenced this pull request Jul 28, 2022
Upstream has dropped the ship and plot workbenches recently. The
patch reflects these changes and drops the USE flags for it.
Note, that the plot module is still available, only the workbench
for separate working with plots has been removed.

See also FreeCAD/FreeCAD#4971 and
FreeCAD/FreeCAD#4900

Bug: https://bugs.gentoo.org/858308#c2
Signed-off-by: Bernd Waibel <waebbl-gentoo@posteo.net>
waebbl added a commit to waebbl/gentoo that referenced this pull request Jul 28, 2022
Upstream has dropped the ship and plot workbenches recently. The
patch reflects these changes and drops the USE flags for it.
Note, that the plot module is still available, only the workbench
for separate working with plots has been removed.

See also FreeCAD/FreeCAD#4971 and
FreeCAD/FreeCAD#4900

Also fixes some typos in pkg_postinst and minor code cleanup.

Bug: https://bugs.gentoo.org/858308#c2
Signed-off-by: Bernd Waibel <waebbl-gentoo@posteo.net>
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Jul 29, 2022
Upstream has dropped the ship and plot workbenches recently. The
patch reflects these changes and drops the USE flags for it.
Note, that the plot module is still available, only the workbench
for separate working with plots has been removed.

See also FreeCAD/FreeCAD#4971 and
FreeCAD/FreeCAD#4900

Also fixes some typos in pkg_postinst and minor code cleanup.

Bug: https://bugs.gentoo.org/858308#c2
Signed-off-by: Bernd Waibel <waebbl-gentoo@posteo.net>
Closes: #26597
Signed-off-by: Sam James <sam@gentoo.org>
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

6 participants