SolidCode / SolidPython

A python frontend for solid modelling that compiles to OpenSCAD
1.1k stars 171 forks source link

Wrong quantities on BoM #162

Closed rockstorm101 closed 3 years ago

rockstorm101 commented 3 years ago

I'm finding this difficulty when using the BoM feature. To make the assembly easier and avoid writing the same code several times I was defining some sub-assemblies within a variable and then adding several of those sub-assemblies onto a base part. Proceeding this way generated the right SCAD code but the wrong BoM. Would it be possible (though I don't see how to be honest with you, so this is maybe more of a rant than an issue itself) to implement a fix for this? The following example illustrates (I hope) the problem. If I were to modify the example file like this:

diff --git a/solid/examples/bom_scad.py b/solid/examples/bom_scad.py
index a059a66..84047fb 100755
--- a/solid/examples/bom_scad.py
+++ b/solid/examples/bom_scad.py
@@ -93,15 +93,17 @@ def doohickey(c):

 def assembly():
+    short_screw = m3_12()
+    nut = m3_nut()
     return union()(
             doohickey(c='blue'),
-            translate((-10, 0, doohickey_h / 2))(m3_12()),
+            translate((-10, 0, doohickey_h / 2))(short_screw),
             translate((0, 0, doohickey_h / 2))(m3_16()),
-            translate((10, 0, doohickey_h / 2))(m3_12()),
+            translate((10, 0, doohickey_h / 2))(short_screw),
             # Nuts
-            translate((-10, 0, -nut_height - doohickey_h / 2))(m3_nut()),
-            translate((0, 0, -nut_height - doohickey_h / 2))(m3_nut()),
-            translate((10, 0, -nut_height - doohickey_h / 2))(m3_nut()),
+            translate((-10, 0, -nut_height - doohickey_h / 2))(nut),
+            translate((0, 0, -nut_height - doohickey_h / 2))(nut),
+            translate((10, 0, -nut_height - doohickey_h / 2))(nut),
     )

The example now produces, as expected I guess, the right SCAD assembly with all the parts on it but the wrong quantities on the BoM:

[...]
Or, Spreadsheet-ready TSV:

Description Count   Unit Price  Total Price link    leftover
M3x16 Bolt  1      € 0.12      € 0.12   http://example.io/M3x160
M3x12 Bolt  1    US$ 0.09    US$ 0.09       0
M3 Nut  1     R$ 0.04     R$ 0.04       
doohickey   1               

Total Cost,    €               € 0.12       
Total Cost,  US$             US$ 0.09       
Total Cost,   R$              R$ 0.04
etjones commented 3 years ago

I think the issue is that the BOM info gets incremented every time the function decorated with @bom_part() gets called. I think it might be possible, somewhere, to increment that code every time the part gets used instead, but I'm not totally sold on the improvement that would create.

One situation where I agree with you is that, especially if we have to make laborious, many-argumented function calls, this could be a hassle. For example:

translate((-10, 0, -nut_height - doohickey_h / 2))(
    nut(nut_sides, nut_color, nut_material, nut_height, extra_arg, other_arg, enough_already)
),
translate((0, 0, -nut_height - doohickey_h / 2))(
    nut(nut_sides, nut_color, nut_material, nut_height, extra_arg, other_arg, enough_already)
),
translate((10, 0, -nut_height - doohickey_h / 2))(
    nut(nut_sides, nut_color, nut_material, nut_height, extra_arg, other_arg, enough_already)
),

In that case, I think a workaround might be:

def make_fancy_nut():
    return nut(nut_sides, nut_color, nut_material, nut_height, extra_arg, other_arg, real_tired_now)

translate((-10, 0, -nut_height - doohickey_h / 2))( make_fancy_nut()),
translate((0, 0, -nut_height - doohickey_h / 2))( make_fancy_nut()),
translate((10, 0, -nut_height - doohickey_h / 2))( make_fancy_nut()),

That ought to yield a correct BOM. But... I take your point that uses of an object rather than function calls is the most natural way to generate a BOM. I'm going to take a look at the BOM code and see if there's an easy win there.

etjones commented 3 years ago

I poked at this a little and decided it was better to store BOM metadata on objects themselves rather than in an ad hoc module-level variable in solid.utils. So... I went ahead and made the change. Usage shouldn't be any different from a user perspective (bill_of_materials() now takes a mandatory OpenSCADObject argument, but that doesn't seem too burdensome), but we now correctly add up different uses of the same object. I've taken your code and added it to the examples/bom_scad.py. Thanks!

etjones commented 3 years ago

Published as v1.0.5 on PyPI

rockstorm101 commented 3 years ago

Wow, thank you very much for looking at this so quickly. I shall test this shortly. And thank you for maintaining this software, I barely write any OpenSCAD code any more since I found this. Great stuff.

(Just a note, are you following SemVer? Cause I would consider the above change as API breaking, even though it feels quite minor)

etjones commented 3 years ago

(Just a note, are you following SemVer? Cause I would consider the above change as API breaking, even though it feels quite minor)

Well, "following" SemVer is probably more accurate ;-). Point taken; this will definitely break any existing code that depends on BOMs. At this point SolidPython development is minor enough that I don't think I'm ready to release a 2.0 for this one feature, although that seems like it would be the technically correct thing to do. But this will definitely persuade me to release a 2.0 sooner rather than later.