OpenSCAD cylinder import fails

[jwueller]

OpenSCAD cylinder import fails

[wwmayer]

You can drop the test code because there isn't really anything to check.

[wwmayer]

Can you provide an example file, please?

[jwueller]

I'll try to get a minimal reproduction extracted from my current project.

[jwueller]

@wwmayer OpenSCAD-Radius2-bug.FCStd.zip

[jwueller]

The PR fixes this by doing what OpenSCAD itself does: If only one radius exists, it's used for both ends of the cylinder.

[wwmayer]

Thanks for the file but it's fairly complex and thus not suitable for a unit test. Do you know a simple way (e.g. a code snippet) to trigger the bug?

[jwueller]

I'll see if I can get it isolated.

[jwueller]

This might be a more fundamental problem. I imported extruder-adaptor-mm.csg.zip, which produced an invalid shape of some kind. I then deleted the offending cylinder024. After a save and load cycle, it then threw the radius error above. So maybe something else got corrupted earlier on during the import process?

[wwmayer]

OK, I found a way to reproduce the error message. The point is to create a document with a Frustum object. Then the document must be saved, closed and restored.

The error message is triggered when restoring the Frustum. Right after restoring the property Radius1 the onChanged() method is triggered but at this point Radius2 is not yet restored.

However, only some error messages are printed but when executing the Python code no exception is raised. This makes it bit more difficult to write some testable code.

import OpenSCADFeatures
doc = App.newDocument()
mycyl=doc.addObject("Part::FeaturePython",'frustum')
r1=2
r2=3
h=4
n=6
OpenSCADFeatures.Frustum(mycyl,r1,r2,n,h)
mycyl.ViewObject.Proxy = 0
doc.recompute()
doc.saveAs("/tmp/openscad.FCStd")
App.closeDocument(doc.Name)
doc = App.openDocument("/tmp/openscad.FCStd")

[jwueller]

I see, in that case my current PR is only dealing with a symptom, and isn't a proper fix. I'm not sure what the underlying problem is, but if you point me towards what has to be fixed I'll attempt it. If you'd rather fix it yourself I'll just close the PR.

[wwmayer]

The main problem is that the class Frustum recreates its geometry directly inside onChanged(). This is considered bad practise and should be avoided. createGeometry() should only be called inside execute().

[jwueller]

I see that this is present in quite a few of those classes. So it's sufficient to just remove onChanged for all of them, essentially?

[wwmayer]

This might be a more fundamental problem. I imported extruder-adaptor-mm.csg.zip, which produced an invalid shape of some kind.

Hmm, I have tried your file and it looks good to me -- independent if I apply your PR or not. I loaded the file into OpenSCAD and there it looks quite the same. In OpenSCAD I exported an STL file, loaded it into FreeCAD and did a visual inspection of the mesh (as actual) and the shape (as nominal) and the highest deviation is less than 0.1 mm.
So, I would say that the loaded object is correct.

Now if you get a broken shape then it's very likely related to something else -- maybe the OCC CAD kernel has some problems with boolean operations.

[wwmayer]

I see that this is present in quite a few of those classes. So it's sufficient to just remove onChanged for all of them, essentially?

Yes, that's what I would suggest. This will also fix the error messages when loading the FCStd file.

[jwueller]

Now if you get a broken shape then it's very likely related to something else -- maybe the OCC CAD kernel has some problems with boolean operations.

Yeah, possibly. I fixed those issues manually in my case, since I was only planning on using the OpenSCAD model as a starting point for further modifications. So it seems that the error this issue is about wasn't caused about an import problem, but entirely by how the model is loaded in instead.

I've removed the onChanged stuff, and added your test. However, I'm not entirely sure what my options are to validate the result, since it doesn't seem to throw in a way I can intercept, as you already noted. Maybe there is a way to read the report view output after loading?

Edit: I pushed those changes to #13559 if you want to take a look. Currently doesn't include the test though.