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

remove plot and ship #4890

Closed
wants to merge 1 commit into from
Closed

Conversation

berndhahnebach
Copy link
Contributor

see #4870

@donovaly
Copy link
Member

I think PartDesign's shaft design wizard uses the plot component. So removing it would be a regression to this feature. Can you please check this before you merge this PR?

@sanguinariojoe
Copy link
Contributor

I think PartDesign's shaft design wizard uses the plot component. So removing it would be a regression to this feature. Can you please check this before you merge this PR?

If it does then it is already failing. Plot module is disabled by default, and the version here is not even working

@donovaly
Copy link
Member

If it does then it is already failing. Plot module is disabled by default, and the version here is not even working

Using the Windows build we provide at our website, you get the shaft design wizard working nicely. When you click on the buttons there, you get very valuable plots of the load, shear force etc.:

FreeCAD_CMzyo6WdkC

So dropping this feature without a replacement would be sad.

@sanguinariojoe
Copy link
Contributor

If it does then it is already failing. Plot module is disabled by default, and the version here is not even working

Using the Windows build we provide at our website, you get the shaft design wizard working nicely. When you click on the buttons there, you get very valuable plots of the load, shear force etc.:

FreeCAD_CMzyo6WdkC

So dropping this feature without a replacement would be sad.

Woha! You are right!

grep -r "Plot" ./
./Mod/PartDesign/WizardShaft/ShaftDiagram.py:        from freecad.plot import Plot
./Mod/PartDesign/WizardShaft/ShaftDiagram.py:        self.win = Plot.figure(title)
./Mod/PartDesign/WizardShaft/ShaftDiagram.py:        self.thePlot = Plot.getPlot()
./Mod/PartDesign/WizardShaft/ShaftDiagram.py:        Plot.xlabel("$%s$ [%s]" % (xname, xunit))
./Mod/PartDesign/WizardShaft/ShaftDiagram.py:        Plot.ylabel("$%s$ [%s]" % (yname, yunit))
./Mod/PartDesign/WizardShaft/ShaftDiagram.py:        Plot.grid(True)
./Mod/PartDesign/WizardShaft/ShaftDiagram.py:        plots = self.thePlot.series
./Mod/PartDesign/WizardShaft/ShaftDiagram.py:            del self.thePlot.series[0]
./Mod/PartDesign/WizardShaft/ShaftDiagram.py:        self.thePlot.update()
./Mod/PartDesign/WizardShaft/ShaftDiagram.py:        self.thePlot.plot(self.xpoints, self.ypoints)
./Mod/PartDesign/WizardShaft/ShaftDiagram.py:        plots = self.thePlot.series
./Mod/PartDesign/WizardShaft/ShaftDiagram.py:        self.thePlot.update()

FEM conversely is using matplotlib directly...

OK, then we need to choose a way to do this (I am opening a forum topic):

  • We keep just the Plot module here (i.e. freecad.plot.Plot), and get the workbench (the tools to manipulate those plots) in its own repo (https://github.com/FreeCAD/freecad.plot)
  • We get everything (module and workbench) here and remove Plot from the Addon Manager

@luzpaz
Copy link
Contributor

luzpaz commented Jun 25, 2021

What about the option that is smart enough to recognize that the user is wanting a feature that requires the Addon Manager to download and install? When the user clicks the function a dialog appears explaining said function has a 3rd party dependency that needs to be d/led and for FreeCAD to restart.

@sanguinariojoe
Copy link
Contributor

What about the option that is smart enough to recognize that the user is wanting a feature that requires the Addon Manager to download and install?

Is it not a bit weird that the user must install external dependencies to use FreeCAD core modules?

On top of that, Plot module itself (not the workbench) is quite simple and requires very little maintenance, so keeping it enabled by default in FreeCAD should be rather harmless.


In the forum someone confirmed that Shaft Wizard is broken unless you enabled Plot during compilation.


I think I will create 2 PRs. One to totally remove Ship and another to remove the Plot workbench and keep just the module (another PR will be required for https://github.com/FreeCAD/freecad.plot afterwards). Hopefully I am convincing you that it is simple enough to keep it in the core as well.

@luzpaz
Copy link
Contributor

luzpaz commented Jun 26, 2021

Is it not a bit weird that the user must install external dependencies to use FreeCAD core modules?

To be clear, I'm saying that core should still function even if plot is external. But if the user clicks a part of the UI in TechDraw that requires plot functionality that it would trigger a dialog saying something like "The external plot workbench is required for this operation, would you like to install it via the Addon Manager? yes/no"

FreeCAD is slated to become a an enormous ecosystem of external workbenches/macros, it makes sense that we start to leverage the Addon manager a lot more because of this

@sanguinariojoe sanguinariojoe mentioned this pull request Jul 1, 2021
7 tasks
@luzpaz
Copy link
Contributor

luzpaz commented Aug 16, 2021

Ship WB has been removed from core. Is now available as external workbench https://github.com/FreeCAD/freecad.ship

@yorikvanhavre
Copy link
Member

I think there is no absolute impossibility in having a core functionality depend onean external module. Indeed it will probably occur more and more in the future. I would say the important point is not to let the user down, so 1) not remove anything that could make a file not open correctly, and 2) if some external component is needed at some point, make it clear and easy to the user. i dont think point 1) applies here, right? then i would simply modify the shaft wizard code to print a nice info if plot is not there, and continue gracefully where it can

@luzpaz
Copy link
Contributor

luzpaz commented Aug 18, 2021

Should we close this PR not that
#4900 has been merged and
#4971 is pending

This PR is obsolete then, correct?

@sanguinariojoe
Copy link
Contributor

sanguinariojoe commented Aug 18, 2021

Totally! I just preserved that to illustrate the discussions on the other PRs

PS I actually cannot close it... No privileged enough

@yorikvanhavre
Copy link
Member

Ok, closing it then!

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