Draft.offset changed its behavior with pull request 2746

[vocx]

Paullee has been working on sketches with walls and a bunch of stuff. See [PR] (Sketch / ArchSketch / DWire) + ArchWall - - - Support Align for Individual Wall Segment.

I believe that in pull request #2746 paullee changed the way the Draft.offset function works. This function depends on DraftGeomUtils.offsetWire which is what was actually changed.

Therefore the Draft tests now fail.

Code: Select all

freecad -t TestDraft

The test for offset is pretty simple, it just creates a rectangle and creates an offset copy.

Code: Select all

rect = Draft.makeRectangle(length, width)
App.ActiveDocument.recompute()

offset = Vector(-1, -1, 0)
obj = Draft.offset(rect, offset, copy=True)

When calling offset, you may receive a message like this.

Code: Select all

Either Part.Wire or Part.Edges should be provided, returning None 

Traceback (most recent call last):
  File "<input>", line 1, in <module>
  File "/opt/freecad-build-vocx/Mod/Draft/Draft.py", line 1688, in offset
    if DraftGeomUtils.hasCurves(newwire) and copy:
  File "/opt/freecad-build-vocx/Mod/Draft/DraftGeomUtils.py", line 120, in hasCurves
    for e in shape.Edges:
AttributeError: 'NoneType' object has no attribute 'Edges'

It also means that the Draft_Offset GUI tool doesn't work as before. It works with lines, but it doesn't work with rectangles and closed shapes.

Paullee, if you are going to make big changes to the code, you need to run the tests to make sure they pass. If they don't pass, you have to change your code so that they pass, or you have to change the tests themselves, adapting them to your new code.

Draft.offset already has a defined behavior, and possibly other code and macros depend on it, so I think we shouldn't change its behavior too much. Paullee maybe you need to write a new function on DraftGeomUtils so that it doesn't change the behavior of the original offset. Or at least you need to test it more extensively before making a pull request.

[paullee]

Ok, should have found the problem.

DraftGeomUtils.offsetwire() originally does not differentiate what is input - whether it is a wire or otherwise. The mentioned PR identify if it is a Part.Wire, or a list of Part.Edge and would accept it. Now noted Draft Wire tool has its default set to MakeFace to True, and it creates a face if user make a close polyline (it has its Display Mode set to Wireframe default). Nothing wrong. Using the Draft Offset over this object, though input shape is face, this is accepted in original DraftGeomUtils.offsetwire(). The mentioned PR is not aware of this usecase. The function is updated to test if the input is a Part.Face and would use its edges to fix the problem. Not sure any other usecases which should be accepted though.

Seem difficult for myself as I tend to post the revised code in the forum hoping someone trained as a programmer and familiar with FC codes could help to have a look whilst using it myself on some works in parallel to test, but comment suggests reviewing at Github is easier. Understood not many has time to test others codes and making a PR add burden to core developer to review, recent tendency is to make something new as Add-on if possible.

In the meantime, I am posting the revised code here, and making a PR mentioning this post in parallel, to see what is best for all parties.

[ EDIT - git commit af75485eb5441dc8181fb2f428ed19f6da8ee334 ]

Screenshot from 2020-01-10 03-07-22.png (189.09 KiB) Viewed 1019 times

[vocx]

...
Seem difficult for myself as I tend to post the revised code in the forum hoping someone trained as a programmer and familiar with FC codes could help to have a look ...

The pull request interface tells you to run the unit tests before submitting the code. You have to do this yourself. This is the reason I refactored the Draft unit tests, so it's easier to check for problems, and assure we don't break previous functionality. In the particular case of Arch, its unit tests aren't very good; they could be re-written following the same style I used in Draft.

Anyway, I submitted pull request #2881 to add a second unit test. Now it tests the offset of an open wire, and of a closed wire, a rectangle. If in the future the offset code changes again, these tests will tell us if there is a problem.

Currently the rectangle test should fail, but after your #2879 is merged, it should pass again hopefully.

[paullee]

The pull request interface tells you to run the unit tests before submitting the code. You have to do this yourself...

Thanks pointing this out ! Finally I note that line of words...

Does it mean I need to compile it and run the 'freecad' command ?

[vocx]

Does it mean I need to compile it and run the 'freecad' command ?

Absolutely.

Whenever I make changes to the code, I re-compile FreeCAD. It's a pain in the behind but that's the proper way to do it to make sure nothing breaks.

As long as you modify only Python code, it's not a big problem because Python files aren't compiled, they are just moved to the installed directory, so the recompilation takes about 1 minute. Of course, if you rebase your branch into the latest master code, the master code may have significant changes to the C++ code; if this happens, then yes, you will have to re-compile for 1 hour or more, depending on how the code was updated.