Moult wrote: ↑Sun Feb 17, 2019 12:02 pm
...
I've created a work-in-progress PR for developing this tool. It is possible to implement it fully quite quickly in a very hacky way, but I want to take this opportunity to refactor some of the draft code to make it more flexible:
https://github.com/FreeCAD/FreeCAD/pull/1975
...
This was merged in
git commit 64d7ed3697.
I just want to mention that with the reorganization of the Draft code that we are doing, I took the liberty of cleaning various snippets that you wrote in
Draft_Move,
Draft_Rotate, and
Draft_Scale. My analysis is this, your code is good but a bit unreadable, with too many list comprehensions and parentheses inside parentheses. My recommendation is to make the code a bit more verbose. I know it sucks to make things verbose, because they are less computationally efficient, but ultimately it's much better for future maintenance. There are many users of Python who aren't expert programmers so for them looking at clean source code is great.
If you want to see the changes we did, look at these files:
Code: Select all
draftguitools/gui_move.py
draftguitools/gui_rotate.py
draftguitools/gui_scale.py
Old. Everything is smushed into a single format().
Code: Select all
for object in self.selected_subelements:
for index, subelement in enumerate(object.SubObjects):
if not isinstance(subelement, Part.Edge):
continue
arguments.append('[FreeCAD.ActiveDocument.{}, {}, {}]'.format(
object.ObjectName,
int(object.SubElementNames[index][len("Edge"):])-1,
DraftVecUtils.toString(self.vector)))
command.append('Draft.copyMovedEdges([{}])'.format(','.join(arguments)))
command.append('FreeCAD.ActiveDocument.recompute()')
return command
New. Each command in a separate line for more clarity, also naming more variables.
Code: Select all
for obj in self.selected_subelements:
for index, subelement in enumerate(obj.SubObjects):
if not isinstance(subelement, Part.Edge):
continue
_edge_index = int(obj.SubElementNames[index][E:]) - 1
_cmd = '['
_cmd += 'FreeCAD.ActiveDocument.'
_cmd += obj.ObjectName + ', '
_cmd += str(_edge_index) + ', '
_cmd += DraftVecUtils.toString(self.vector)
_cmd += ']'
arguments.append(_cmd)
all_args = ', '.join(arguments)
command.append('Draft.copyMovedEdges([' + all_args + '])')
command.append('FreeCAD.ActiveDocument.recompute()')
return command
Obviously this is more verbose and maybe not as fast if we measure in microseconds, but I think it's more maintainable. Since this is graphical code, it doesn't need to be extremely fast, the graphical event loop anyway is slow. Human reaction times are in the order of 100 milliseconds. Anything faster than that we will not notice. We want maximum efficiency only with code that should run completely from the terminal.
In other cases there are list comprehensions for lists of maybe 10 elements (vertices in a polygon, for example). Sure the list comprehension is fast, but for this case we don't save significant time if we just do a good old loop. It would only make sense if we need to process thousands of elements, but in this case, most time is spent in the GUI processing.
I also noticed some issues with the functions that modify vertices, edges, and perform copy and move, inside Draft.py: moveVertex, moveEdge, copyMovedEdges, copyMovedEdge, etc. However, these issues I will address once most Draft objects have been moved by Carlo to individual modules. Then it will be much easier to handle, because right now, modifying a 7000 line module is a bit hard.
I know that since you made these changes to Draft, you moved your focus more to BlenderBIM. I think at some point you basically said "do whatever you want to my code, or remove it, whatever". That's fine. I don't think we need to get rid of your code, but I think it makes sense to keep it clean and maintainable.