nophead / NopSCADlib

Library of parts modelled in OpenSCAD and a framework for making projects
GNU General Public License v3.0
1.22k stars 160 forks source link

Suggestion - remove vitamin array from each vitamins.scad file and move to test #200

Open martinbudden opened 3 years ago

martinbudden commented 3 years ago

Each vitamins.scad file contains an array of those vitamins used for testing. For example vitamins/nuts.scad contains the assignment

nuts = [M2_nut, M2p5_nut, M3_nut, M4_nut, M5_nut, M6_nut, M8_nut];

and the nuts[] array is only used in test/nuts.scad. Moving the array to the test file removes overhead from the build system and will speed up builds (I haven't measured how much).

For screws.scad I suggest that we create a utils/screws.scad and move screw_lists[], screws[], find_screw and alternate_screw there - I expect that will substantially improve build times`.

@nophead , if you agree this is a good plan, I will create the PR.

nophead commented 3 years ago

It is only one more list in each file full of lists. I doubt it has much effect. Moving it to the test file means both have to be edited to add a new vitamin.

martinbudden commented 3 years ago

I doubt it has much effect. Moving it to the test file means both have to be edited to add a new vitamin.

Yes, this is a trade of between build time and convenience when adding a new vitamin. I guess I need to do some measurements to see if there actually is any noticeable saving.

martinbudden commented 3 years ago

It is only one more list in each file full of lists

While this is true, it is a list of lists. I have no idea of the underlying OpenSCAD method of copying lists, if OpenSCAD does a shallow copy then this isn't so bad, but if it does a deep copy it will double the time to assign the elements to the lists.

nophead commented 3 years ago

I think list copying got optimised in OpenSCAD some time ago by thehans.

nophead commented 3 years ago

The test lists could be made into functions. That way they would just get parsed, and only constructed when used by the test routines.

I don't know if it would make a significant difference or not. I find with a single file project it reasonably quick because all these things are only calculated once. It becomes a problem with a multi file project if the project's files are are used in each other.

martinbudden commented 3 years ago

I'm looking at doing some benchmarks, but I am getting significant variance in build times - ie if I run the same build twice I can get variances of over 10% - not sure why.

nophead commented 3 years ago

I think it depends what else the PC is doing. The times are real time, not CPU time.

martinbudden commented 3 years ago

I ran them with nothing else running, but I guess my PC could have started running some background process like checking for updates or viruses.

nophead commented 3 years ago

I get some pretty random results even when my PC is not doing anything. For example I just removed some unused dependencies on screws.scad and some times got a little longer, which makes no sense.

martinbudden commented 3 years ago

It doesn't seem I'll be able to get any reliable benchmarks for this, so I'll make one more attempt to convince you.

Some of my projects have build times of several hours, and any reduction in this, even it is only minutes, is worthwhile from my perspective. I appreciate that build times are dominated by the performance of OpenSCAD itself, but that doesn't mean that changes to the library are not significant.

Generally speaking, it is accepted practice that release code can contain hooks for testing, but these should be avoided if there are alternatives.

The presence of the vitamins arrays in the vitamins file is a convenience for testing that could easily be moved into the test file itself. The downside of this is, as you point out, is that the test file has to be edited when you add a new vitamin - but this is a only a minor inconvenience, and sometimes you need to edit the test file anyway (for example to change layout).

nophead commented 3 years ago

The way to speed up big projects is to include only core.scad and add what other includes are needed and use the rest. Use include to combine the files of your project instead of use. Make sure your stls use the stl() child method to use generated STLs.

If lists are handled efficiently in OpenSCAD, which I think they are, then I don't think these will make any difference. Each vitamin list already exists, so I think making the list of them is simply gathering the pointers and allocating one small additional list.

If you can't see a noticeable improvement, i.e. the difference in times is lost in the noise, then what it the point?

Did you test it in a project or the library. When I removed some screw dependencies the library tests didn't improve but it seemed to knock minutes of a project.

martinbudden commented 3 years ago

The way to speed up big projects is to include only core.scad and add what other includes are needed and use the rest. Use include to combine the files of your project instead of use. Make sure your stls use the stl() child method to use generated STLs.

I already do all of that, except I generally combine my project files using use rather and include. Here's an example of a typical .scad file:

include <global_defs.scad>

include <NopSCADlib/core.scad>
include <NopSCADlib/vitamins/pillar.scad>
include <NopSCADlib/vitamins/pcbs.scad>
include <NopSCADlib/vitamins/psus.scad>
use <NopSCADlib/vitamins/sheet.scad>

use <printed/extruderBracket.scad>
use <printed/PSU.scad>
use <printed/WiringGuide.scad>

use <utils/bezierTube.scad>
use <utils/FrameBolts.scad>
use <utils/printheadOffsets.scad>

use <vitamins/bolts.scad>
use <vitamins/nuts.scad>

use <Parameters_Positions.scad>
use <Parameters_CoreXY.scad>
include <Parameters_Main.scad>

I use use for combination since it allows variable that don't pollute the global namespace - would replacing those uses with includes speed things up? Why?

If you can't see a noticeable improvement, i.e. the difference in times is lost in the noise, then what it the point?

I didn't say I can't see a noticeable improvement, I said I can't generate reliable benchmarks. As I mentioned I can get large variations in running my benchmarks, sometimes over 20%. Let's say my change makes a 5% improvement - then I'd have to run the benchmarks a large number of times to see the improvement above the noise.

I am using one of my projects, not the library, to do the testing.

nophead commented 3 years ago

When you use use then any call into that file for functions or modules causes all the file scope variables to be reevaluated. That includes all the includes from the library, so all those lists get evaluated over and over again. In my 3D printer it is hundreds of thousands of times and takes 2 minutes to do F5 on main.scad when nothing has changed. That is with a lot of files included but a few are still used for the reasons you mentioned.

nophead commented 3 years ago

The worst list by far is the screw list because it is included almost everywhere, is the longest and is created nested and then flattened. It tried making it a function and saw no clear improvement, it is lost in the noise. Moving it out would be a breaking change, so it doesn't seem worth it for no noticeable benefit.

Most of the rest are short lists and don't seem to take a significant amount of time to generate, even though they are copies of the rest of the lists in the same file, they seem to be quick to produce.

nophead commented 3 years ago

My largest project takes 1 hour and 15 minutes to build, 15 minutes faster than the last time I built it, but I don't remember when that was or on which machine.

It is a tool changing 3D printer configured with 4 extruders. It has 33 scad files with about 7000 lines of code. There are 58 assemblies, 1833 vitamins, 156 printed parts and 32 routed parts.

main_assembled

I might finish it one day!

How does that compare to the complexity of your projects that are taking hours to build?

martinbudden commented 3 years ago

Nice printer!

My two most complicated projects are the MaybeCube with 33 assemblies, 576 vitamins, 53 printed parts, and 3 routed parts and the BabyCube with 23 assemblies, 307 vitamins, 37 printed parts, and 1 routed part.

I have considerably fewer vitamins than you since I don't use threaded inserts, don't use nuts (except t-nuts) having opted for using self taping screws pretty well throughout.

(I also have a proof of concept of the MaybeCube using the Jubilee Toolchangeer, see https://github.com/martinbudden/MaybeCube/blob/main/JubileeToolchanger/assemblies/JubileeToolchanger_assembled.png.

I don't often build these from scratch, so am not entirely sure about what my build times are. I've just done a re-org as per your suggestion of using include rather than use and this does seem to make a big difference.

nophead commented 3 years ago

Yes the bug where OpenSCAD recomputes every file scope variable instead of just the ones that depend on $variables has a huge impact. Looks like rather than fix it they will stop $variables working instead.

There is a branch of OpenSCAD with a persistent cache: https://github.com/openscad/openscad/tree/persistent-cache

I haven't tried it but I would think it would make builds much faster but it seems to have gone stale.

My 3D printer is mainly slow because it has a pathological dependency problem. There is a shelf bracket that is screwed to four of the panels. It has screw positions that have to avoid things on the panels, so it depends on them, but then they depend on it to get the screw hole positions.

shelf_bracket_assembled

I probably use too many inserts. I just made a project where I just used machine screws into tapped pillars and they have been in and out a few times. I used the twisted poly_holes to make the pilot holes. The M2 ones don't even need tapping, they just screw in.

box_top_assembly

There are 14 PCB / veroboard assemblies in this project I am working on.

martinbudden commented 3 years ago

My 3D printer is mainly slow because it has a pathological dependency problem. There is a shelf bracket that is screwed to four of the panels. It has screw positions that have to avoid things on the panels, so it depends on them, but then they depend on it to get the screw hole positions.

I think getting the screw positions correct is probably one of the most difficult aspects of designing an assembly. And if you need to change it then you have to reprint both the parts that are fastened together. I now have much more sympathy for companies that glue their products together.

To deal with the dependency problem I've created a file utils/holepositions.scad which is included by parts. Rather than positioning screw holes to avoid items on a part, I position items on a part to avoid the screw holes. It seems a bit mad, but once I elected to do this I found I became more productive. The process is something like this :

  1. for strength/rigidity these two parts need (say) 4 screws to fasten them together, so I'll place them in these positions.
  2. then I design the rest of the parts around the hole positions.

Of course there is some iteration, and the hole positions may need to be revised, but I have found it to be generally a good way of working.

nophead commented 3 years ago

Yes most of my brackets decide where the holes should be and then the mating parts get their matching holes by depending on the bracket. This one is a special case because it has to avoid other things mounted on the panels (which are four different assemblies), such as fans, the mains inlet and the Z spine. It also has cut outs for those and an ethernet socket and a ventilation grill. And it also has to be able to be cut into sections to fit the machine printing it, hence the double screws.

The back panel layout depends on the Z axis, which depends on the hbot at the top and the left panel depends on the electronics in the base. So the shelf bracket depends on pretty much all of the machine and the machine printing it but the Z_axis and four sides also depend on it for screw holes.

image

I have this function to decide the screw positions, returned as transformation matrices:

function avoid(obstacles, x, dir, i = 0) = let(o = obstacles[i], r = shelf_boss_r + eps) is_undef(o) ? x
                                                                : x + r > o.x - o.y && x - r < o.x + o.y ? o.x + dir * (o.y + r)
                                                                : avoid(obstacles, x, dir, i + 1);

function shelf_screw_positions() = !is_undef($shelf_screw_positions) ? $shelf_screw_positions : echo("computed")
let(sw = spine_width(), sd = spine_depth(),
    bc = box_intrusion + box_bezel_clearance(box_type),
    r = shelf_boss_r + eps,
    left_obstacles = [[controller_pos().y, fan_guard_width(electronics_fan) / 2 + min_clearance], [expander_pos().y, fan_guard_width(electronics_fan) / 2 + min_clearance]],
    back_obstacles = [[inlet_x + inlet_w / 2, inlet_w / 2], [z_axis_x, sw / 2]]
   ) [
    // Z axis
    translate([z_axis_x + sw / 2, box_depth / 2 - sd + spine_clearance + r])                                       * rotate([-90, 0, 90]),
    translate([z_axis_x - sw / 2, box_depth / 2 - sd + spine_clearance + r])                                       * rotate([-90, 0, -90]),

    // Back
    for(i = [1 : 1 : shelf_cols - 1], j = [-1, 1])
        translate([avoid(back_obstacles, -box_width / 2 + i * box_width / shelf_cols + j * r, j), box_depth / 2])  * rotate([-90, 0, 0]),

    // Right
    translate([box_width / 2,                box_depth / 2 - bc - r])                                              * rotate([-90, 0, -90]),
    translate([box_width / 2,               -box_depth / 2 + bc + r])                                              * rotate([-90, 0, -90]),
    for(i = [1 : 1 : shelf_rows - 1], j = [-1, 1])
        translate([box_width / 2, -box_depth / 2 + i * box_depth / shelf_rows + j * r])                            * rotate([-90, 0, -90]),

    // Front
    for(i = [1 : 1 : shelf_cols - 1], j = [-1, 1])
        translate([-box_width / 2 + i * box_width / shelf_cols + j * r, -box_depth / 2])                           * rotate([-90, 0, 180]),

    // Left
    translate([-box_width / 2,               box_depth / 2 - bc - r])                                              * rotate([-90, 0, 90]),
    translate([-box_width / 2,              -box_depth / 2 + bc + r])                                              * rotate([-90, 0, 90]),
    for(i = [1 : 1 : shelf_rows - 1], j = [-1, 1])
        translate([-box_width / 2, avoid(left_obstacles, -box_depth / 2 + i * box_depth / shelf_rows + j * r, j)]) * rotate([-90, 0, 90]),

];

It is so expensive to call that I cache the result in a $variable in main and if it finds it set it just returns the $variable. So it still works if I open an assembly without main, it just calculates it once but when main calls all the assemblies it still only calculates it once.

My main.scad looks like this:

main = true;

include <global.scad>
include <positions.scad>
$shelf_screw_positions = shelf_screw_positions();
use <extruder.scad>
use <grabber.scad>
use <NopSCADlib/printed/door_hinge.scad>
use <display_case.scad>

include <door.scad>
include <sides.scad>
include <back.scad>
include <base.scad>
include <top.scad>
include <front.scad>
include <shelf.scad>
include <NopSCADlib/printed/box_assembly.scad>

Stand alone assemblies like the extruder are used but all the big assemblies that depend on global positions are included. To make them work stand alone but not generate objects when included I have this:

if(is_undef(main) && $preview)
    !box_back_assembly();

It is truly horrible trying to handle the interdependencies of big machines made with panels. Probably easier with extrusions as you don't need many holes.

martinbudden commented 2 years ago

I've now got some benchmarks for this.

I've make a change where I've moved the test arrays out of screws, nuts, washers and rails, so not everything, but ones which are widely used in my project. Additionally I've moved the screw utility functions find_screw and alternate_screw and the screws arrays into utils/screws.scad which I've included (instead of vitamins/screws.scad in core.scad. To test run 3 of my test scripts. I've run the tests twice. The results are as follows:

Using current arrangement:

  45.8   8.5 tests/BackFace.scad
  57.7   9.1 tests/Base.scad
 136.6  14.1 tests/Main.scad
 240.0  31.7 TOTAL

  47.0   9.1 tests/BackFace.scad
  59.8   7.7 tests/Base.scad
 141.9  12.5 tests/Main.scad
 248.7  29.4 TOTAL

Having moved test arrays into test files:

  37.9  -7.9 tests/BackFace.scad
  52.1  -5.6 tests/Base.scad
 129.4  -7.2 tests/Main.scad
 219.3 -20.7 TOTAL

  39.0  -7.9 tests/BackFace.scad
  50.6  -9.2 tests/Base.scad
 128.4 -13.5 tests/Main.scad
 218.0 -30.7 TOTAL

I'm getting about a 10% improvement, more in some cases. If all the test arrays were moved, then this would be even more.

nophead commented 2 years ago

How does moving the lists from screws.scad to utils/screws.scad help? Doesn't that still cause the screw lists to be included everywhere but now by an extra level of include?

martinbudden commented 2 years ago

It means that any code that includes NopSCADlib/core.scad continues to work, ie it doesn't break existing code.

To get the performance benefit you need to include NopSCADlib/utils/core/core.scad and NopSCADlib/vitamins/screws.scad instead.

Anyone already directly including NopSCADlib/vitamins/screws.scad will get the benefit immediately, although they will need to change to NopSCADlib/utils/screws.scad if they are using either find_screw or alternative_screw.

My BabyCube code took about 15 minutes of rework to get the benefit, so the cost isn't onerous.

I've added a "FOR DISCUSSION" PR, https://github.com/nophead/NopSCADlib/pull/231, so you can see what I've done.

nophead commented 2 years ago

To get the performance benefit you need to include NopSCADlib/utils/core/core.scad and NopSCADlib/vitamins/screws.scad instead.

I don't like that as it isn't the recommended way of using the library. I think if utils/screws.scad was used in core.scad then it would not have any overhead unless find_screw() or althernative_screw() was called. Then it would construct the lists on each call.

martinbudden commented 2 years ago

So do you mean to change core.scad to:

//
// Include this file to use the minimum library plus screws, nuts and washers
//
include <utils/core/core.scad>
//
// Fasteners used by a lot of other vitamins
//
include <vitamins/screws.scad>
use <utils/screws.scad>

That's fine by me. The main gist of this PR is moving the test arrays from the vitamins files to the test files - that's what provides the speedup.

nophead commented 2 years ago

Yes that is backwards compatible but also should give the speed up by default without people having to change their projects. Only if you use a lot of find_screw() calls would it be slower and then you could include utils/screws to perhaps get it faster.

What has changed since the last try when we both got inconclusive results?

martinbudden commented 2 years ago

The problem when I was testing before was that I was getting a lot of variation in my test times, which I believe were due to various background processes doing things on my PC. I made a bit of an effort to stop these things happening - there are still some variations in my test times, but they are small enough to show the underlying changes.

nophead commented 2 years ago

I will see if I can recreate it with an old laptop doing nothing else