[Discussion] Splitting Draft tools into their own modules

A forum dedicated to the Draft, Arch and BIM workbenches development.
vocx
Posts: 3357
Joined: Thu Oct 18, 2018 9:18 pm

[Discussion] Splitting Draft tools into their own modules

Postby vocx » Thu Aug 15, 2019 6:39 pm

Inspired by this thread Your Priorities for the Draft Workbench, I think it's time that Draft tools are moved outside Draft.py and DraftTools.py, and put in their own modules, like DraftLine.py, DraftWire.py, DraftRectangle.py, etc. This is done precisely in the Arch Workbench, and in Draft it's already somewhat on the way, see git commit 5ee99ca, git commit a4e2df1 and git commit 16c26cb.

See pull request #2429. This isn't suitable for merging, I just created it so people can see approximately the code changes that are needed, and the diffs against the current code.

Some of the things that are needed are
  • New module to hold the commands, for example, DraftWire.py.
  • Remove the makeCommand() from Draft.py, for example, makeLine() and makeWire().
  • Remove the object class and its respective view provider, for example, _Wire and _ViewProviderWire, from Draft.py. These two classes are now defined in the new module, for example, DraftWire.py.
  • Inside Draft.py the new module DraftWire is imported, and the function makeWire() is made available, as they may be used by other functions, and they also should be available in the Draft namespace.
  • The _Wire and _ViewProviderWire class are also imported in Draft.py, as these need to be present in the Draft namespace for compatibility with previous versions. Without these classes, it isn't possible to recreate the shapes when a file is re-opened.
  • Remove the Wire class from DraftTools.py. This class is a Gui Command, meaning, the command run when the button in the Draft toolbar is pressed.
  • Remove the FreeCADGui.addCommand("Draft_Wire", Wire()) from DraftTools.py, as this is now added in the new module, DraftWire.py.
  • Update the CMakeLists.txt file with the new module DraftWire.py so that when FreeCAD is built, the file is installed (moved) to the appropriate directory (it's a Python file, so it isn't compiled like a C++ source).
  • In Draft.py auxiliary functions and classes need to be moved to before the import DraftWire instructions. Otherwise, they aren't found. Maybe this isn't needed if new modules with auxiliary functions are created.
Merging pull request #2429 and compiling FreeCAD works. Since it's Python code it doesn't crash the system. However, the error is displayed when you load the Draft Workbench. It also fails the unit tests.

Code: Select all

./bin/FreeCAD --run-test TestDraft

Code: Select all

======================================================================
ERROR: TestDraft (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: TestDraft
Traceback (most recent call last):
  File "/usr/lib/python3.6/unittest/loader.py", line 153, in loadTestsFromName
    module = __import__(module_name)
  File "/opt/freecad-build-vocx/Mod/Draft/TestDraft.py", line 26, in <module>
    import FreeCAD, os, unittest, FreeCADGui, Draft
  File "/opt/freecad-build-vocx/Mod/Draft/Draft.py", line 462, in <module>
    import DraftWire
  File "/opt/freecad-build-vocx/Mod/Draft/DraftWire.py", line 48, in <module>
    from DraftTools import getPoint
  File "/opt/freecad-build-vocx/Mod/Draft/DraftTools.py", line 562, in <module>
    from DraftWire import Line
ImportError: cannot import name 'Line'
This is possibly a circular dependency. Draft is imported, which imports the new DraftWire to define Line and Wire; this DraftWire needs some utilities from Draft itself like typecheck(), getParam(), dimSymbol(), etc. Moreover, DraftWire also needs some utilities from DraftTools, like the Creator class, as well as a few auxiliary tools getPoint() and getSupport(); but DraftTools also imports DraftWire because it needs to import Line, as it is used by other tools, like BSpline.

I don't know what the proper solution is but maybe a new module should be created like DraftAuxTools.py that includes all small utilities, but doesn't actually define a working tool like Line, Circle, Ellipse, etc. In similar fashion, another module could be added DraftBaseObjects.py to define the auxiliary classes, like _DraftObject, _ViewProviderDraft, Creator, DraftTool, etc.

I believe this is done in the Arch Workbench, as ArchComponent.py, ArchCommands.py, etc., define auxiliary tools that the other objects need.

I opened this thread to discuss the issue. Obviously, I could get to work and try to hack a solution, moving the tools to new modules. However, since the codebase is massive (Draft.py has 6982 lines and DraftTools.py has 5676, and I need to add about 2000 more lines of documentation) maybe Yorik has a better idea of what can be moved first and how.
Always add the important information to your posts if you need help.
To support the documentation effort, and code development, your donation is appreciated: paypal.
User avatar
kkremitzki
Posts: 1929
Joined: Thu Mar 03, 2016 9:52 pm
Location: Texas

Re: [Discussion] Splitting Draft tools into their own modules

Postby kkremitzki » Fri Aug 16, 2019 12:33 am

We probably shouldn't have modules over 1k lines, and we definitely shouldn't have ones over 5k lines, so in that regard, +1 from me.
Like my FreeCAD work? I'd appreciate any level of support via Patreon, Liberapay, or PayPal! Read more about what I do at my blog.
triplus
Posts: 9278
Joined: Mon Dec 12, 2011 4:45 pm

Re: [Discussion] Splitting Draft tools into their own modules

Postby triplus » Fri Aug 16, 2019 1:41 pm

On the cons side all existing scripts out there would likely stop working after such attempt. Therefore there needs to be some additional value, other than just cosmetics and personal taste in such effort. Like for example combining the effort with porting to new import style or something like that. But that too would be hard to justify ATM. We once changed an API for creating lines/segments. And such API change was detected in discussions ever since.

P.S. Something like Draft NEXT effort, if there ever will be one, is where such discussion can materialize and for them to make more sense.
vocx
Posts: 3357
Joined: Thu Oct 18, 2018 9:18 pm

Re: [Discussion] Splitting Draft tools into their own modules

Postby vocx » Fri Aug 16, 2019 4:10 pm

triplus wrote:
Fri Aug 16, 2019 1:41 pm
On the cons side all existing scripts out there would likely stop working after such attempt.
...
Why would they stop working?

As far as I can tell, all public functions and classes would still be provided in the Draft namespace, that is

Code: Select all

Draft.makeLine()
Draft._Wire
Draft.makeWire()
...
So there would not be breaking of old scripts. That's exactly what already happened with DraftEdit.py and DraftLayer.py (formerly Draft VisGroup). These two were split out of Draft.py and they keep working as intended.

Maybe, maybe in the more distant future there would be a need to re-write some parts, or rename the classes, and that could break compatibility, but I don't envision that happening right now.
Always add the important information to your posts if you need help.
To support the documentation effort, and code development, your donation is appreciated: paypal.
User avatar
kkremitzki
Posts: 1929
Joined: Thu Mar 03, 2016 9:52 pm
Location: Texas

Re: [Discussion] Splitting Draft tools into their own modules

Postby kkremitzki » Fri Aug 16, 2019 5:16 pm

Yeah, preserving the import functionality is far from difficult, one can for example add to __init__.py:

Code: Select all

from module_we_split_out1 import *
...
from module_we_split_outN import *
Like my FreeCAD work? I'd appreciate any level of support via Patreon, Liberapay, or PayPal! Read more about what I do at my blog.
User avatar
yorik
Site Admin
Posts: 11717
Joined: Tue Feb 17, 2009 9:16 pm
Location: São Paulo, Brazil
Contact:

Re: [Discussion] Splitting Draft tools into their own modules

Postby yorik » Fri Aug 16, 2019 5:24 pm

I think this is pretty much the right direction @vocx

Basically I think it can indeed follow the same philosophy of Arch: "Everything about rectangles goes into a same file": that includes the object definition, the make() function and the command class

Points to look at:

1) Backwards compatibility: If the Line object is moved to DraftLine, I think in Draft we need something like: "_Line = DraftLine.Line" I think this can be in a clean block somewhere (I started that the the begin of Draft.py) Maybe there is a better way to do it, though.

2) _ classes: At the early beginnings, I used that notation to try to simplify the Draft API. However, these classes that begin with _ don't simplify anything, they just don't appear in doxygen or sphynx :/ which is the opposite of what we want... So we might use the opportunity to fix that...

3) Ideally any scriptwriter out there should just do "import Draft" and not care about anything else, everything should be there: make() functions and object classes. I don't think any script out there uses DraftTools.py, so I think we can do pretty much what we want there. At the end I suppose it could still contain command classes for the very simple ones, and functions used by all other commands. That is, DraftLine should import DraftTools, but DraftTools shouldn't import DraftLine. That also means commands should be created when "import Draft", and we should progressively remove "import DraftTools" from all over the place...

4) Of course, keep in mind that all this should still work in non-GUI mode (no FreeCADGui or Qt or coin import at file head) and FreeCAD should start without importing heavyweight modules like Part or QtGui

About the issue with Wire and BSpline, I think the most higher-level classes should be kicked out first (Bspline, bezier...) then after that you'll be able to take wire out, etc... Another solution could be to keep line, wire and curves together, but that would be a bit against what we're doing here...

How does that sound?
vocx
Posts: 3357
Joined: Thu Oct 18, 2018 9:18 pm

Re: [Discussion] Splitting Draft tools into their own modules

Postby vocx » Fri Aug 16, 2019 6:05 pm

yorik wrote:
Fri Aug 16, 2019 5:24 pm
1) Backwards compatibility: If the Line object is moved to DraftLine, I think in Draft we need something like: "_Line = DraftLine.Line" I think this can be in a clean block somewhere (I started that the the begin of Draft.py) Maybe there is a better way to do it, though.
Okay. Just a nitpick here, for anybody reading, there is no _Line class, because the Line tool just calls the Wire tool. That is, a Line is identical to a Wire, but with two points only.
2) _ classes: At the early beginnings, I used that notation to try to simplify the Draft API. However, these classes that begin with _ don't simplify anything, they just don't appear in doxygen or sphynx :/ which is the opposite of what we want... So we might use the opportunity to fix that...
Yes, I prefer that public classes don't have underscore. Underscore should be used for random variables or functions that we don't need to make public.
... That is, DraftLine should import DraftTools, but DraftTools shouldn't import DraftLine. That also means commands should be created when "import Draft", and we should progressively remove "import DraftTools" from all over the place...
Okay, this makes perfect sense. When I suggested above using an auxiliary module DraftBaseTools.py, I basically meant that we could use a temporary module while DraftTools.py is being refactored, so as to preserve compatibility as much as possible.
4) Of course, keep in mind that all this should still work in non-GUI mode (no FreeCADGui or Qt or coin import at file head) and FreeCAD should start without importing heavyweight modules like Part or QtGui
Yes, I would pay attention to this as well. While I was testing this, I noticed that I could import Draft with no issues. I even created a few Lines and Wires from the terminal. However, when I tried switching to Draft from the graphical interface, then the system threw import errors and undefined variables.

I need to know more about the loading process of the workbenches. When the graphical interface is loaded, does it call Init.py and InitGui.py only? Does it call anything else?
About the issue with Wire and BSpline, I think the most higher-level classes should be kicked out first (Bspline, bezier...) then after that you'll be able to take wire out, etc...
Duh, I didn't think of this myself. I initially tried splitting the Line and Wire tools because I thought they were simpler. But you are right, we should split the more complex features first, because they inherit from the simpler ones. So the simpler ones should go last, not first.
How does that sound?
Sounds good to me.

I basically just wanted to avoid duplicate work, because I don't know if you or somebody else (carlopav?) is also working on this. I wouldn't want to do something and then realize the same exact job is being done by somebody else.
Always add the important information to your posts if you need help.
To support the documentation effort, and code development, your donation is appreciated: paypal.
User avatar
DeepSOIC
Posts: 7485
Joined: Fri Aug 29, 2014 12:45 am
Location: Saint-Petersburg, Russia

Re: [Discussion] Splitting Draft tools into their own modules

Postby DeepSOIC » Fri Aug 16, 2019 7:00 pm

vocx wrote:
Thu Aug 15, 2019 6:39 pm
This is possibly a circular dependency. ...I don't know what the proper solution ...
I think, it can be solved by never using the style "from Draft import something" in any other module that is used by Draft.py in that very import style (from DraftSomething import whatever). Instead, just use "import Draft", and refer to something as Draft.something.
triplus
Posts: 9278
Joined: Mon Dec 12, 2011 4:45 pm

Re: [Discussion] Splitting Draft tools into their own modules

Postby triplus » Fri Aug 16, 2019 9:09 pm

vocx wrote:
Fri Aug 16, 2019 4:10 pm
Why would they stop working?
Such things usually happen if you for example move a function to another file (module). But indeed likely some sort of "shim approach" could be used to preserve backwards compatibility. Mainly for imports in existing scripts to continue working.
carlopav
Posts: 839
Joined: Mon Dec 31, 2018 1:49 pm
Location: Venice, Italy

Re: [Discussion] Splitting Draft tools into their own modules

Postby carlopav » Sat Aug 17, 2019 11:03 am

vocx wrote:
Fri Aug 16, 2019 6:05 pm
I basically just wanted to avoid duplicate work, because I don't know if you or somebody else (carlopav?) is also working on this. I wouldn't want to do something and then realize the same exact job is being done by somebody else.
Hi Vox, thanks to handle such effort, I think this work will be really helpful to boost comprehension and transparency of the code.
At the moment I'm not working on similar tasks: everything i did and i'm doing is strictly confined inside DraftEdit.py: there's still lot to do there, i'd like the tool to become more and more userfriendly and powerful, and at the end I don't think i'm skilled enough to carry one some tasks like splitting tools into modules. So go on!
I was thinking... also Draft Edit became huge (currently +1000 lines), and it contains lot of code to gather editpoints vectors and to update objects with new points positions. Is it something that could be also contained in each object module, so DraftEdit could just call object.giveMeEditPoints() and object.updateVertex(vertexIndex, FreeCAD.Vector) ?
follow my experiments on BIM modelling for architecture design