Refactor to make SCAD files more modular

[lastcoolnameleft]

This is NOT ready for merge, but instead an attempt to break up the monolithic (~1000+ SLOC) splitflap.scad into separate modules and elicit feedback for this refactor.

There should be 0 functional change to splitflap.scad, just code moved around.

The goal for this refactor is to make the scad files more "readable" for beginners to the project and to be able to split the parts into separate files so that we could iterate on them easier for 3d-printing.

For example, see 3d/splitflap_3dprinter.scad which has a simplified version of the parts needed to 3d-print. This file could later be expanded to include the frame, which might vary depending on the material or design (e.g. without the captive nuts)

Again, this PR is just to get feedback if we should proceed down this path or if the value isn't there.

[scottbez1]

Yeah I think this makes a lot of sense, and the initial modules you selected to extract seem like good choices.

The only potential hiccup I see is how some of the scripting works with the refactored changes. I haven't looked too closely at the exact reasons, but it seems like the positioning of components may not be working?

The export hit a potential bug in the SVG post-processing

Traceback (most recent call last):
  File "3d/scripts/generate_2d.py", line 86, in <module>
    redundant_lines, merged_lines = processor.remove_redundant_lines()
  File "/home/runner/work/splitflap/splitflap/3d/scripts/svg_processor.py", line 201, in remove_redundant_lines
    path.attributes['d'] = filtered_path.d()
  File "/usr/local/lib/python3.8/dist-packages/svg/path/path.py", line 404, in d
    end = self[-1].end
  File "/usr/local/lib/python3.8/dist-packages/svg/path/path.py", line 301, in __getitem__
    return self._segments[index]
IndexError: list index out of range

but looking at the partial output it seems like that bug may have been triggered due to some behavioral change in the 2d export process causing components to stack on top of each other?

[dmadison]

Did you forget to commit all of your changes? I don't see common.scad, and on my machine the main splitflap.scad file opens with only the spool geometry and 224 warnings 👀

[lastcoolnameleft]

Thanks @dmadison ! I sure did. Just added and pushed.

Hopefully this will fix the issues @scottbez1 encountered too as there shouldn't be any behavior changes. If not, can you let me know what command you ran to get that error?

[dmadison]

Yup, that fixed it 👍 . Renders fine on my machine and the 2D output looks good as well.

[lastcoolnameleft]

I finished off the refactoring of this exercise. There should be no functional change, just splitting the code into different files and making it more modular. I took some best-guesses at how it should be organized which I'm not attached to.

I also found some dead code, which I didn't want to remove as that's out-of-scope for this PR, but could if it's worthwhile.
https://github.com/lastcoolnameleft/splitflap/blob/refactor/3d/splitflap.scad#L107-L116

I also split the 2d and 3d rendering parts into separate files (splitflap_2d.scad and splitflap_3d.scad). My thought is that there could be a specific files the different styles (e.g. laser cut for SVG, 3d print for STL, fully rendered for visualization, anything else that comes down the line)

It's splitflap.scad is now down to 125 lines (woah!) and feels much easier to mentally digest and hopefully allow people's forks to be easily merged back into this main repo.

[scottbez1]

Wow, this looks great, thanks so much. Is this ready for review & merge? If so, I'll try to review ASAP (by end of weekend at the latest) to avoid the inevitable merge conflicts if such a large refactor is left waiting.

One thing I noticed from my quick glance over the diff was the 3d-printed file -- I think it would be best to omit that for now to keep the refactor more "pure", and also to avoid confusion once this is merged since I think that is not a complete design yet?

[scottbez1]

I think there are a lot of dependency issues that need to be resolved.

I'm seeing warnings in a lot of different files (when loaded independently) due to unresolved dependencies which are implicitly satisfied when the files are (transitively) included from the top level splitflap.scad file. That makes this a problematic refactor, as the refactored files are not usable independently, so there are now hidden dependencies between files that rely on correct ordering of inclusion.

For example, hardware.scad has WARNING: Ignoring unknown function 'color_multiply', in file hardware.scad, line 36. but there's actually more dependency issues that appear if you try to use any of the modules defined in that file, e.g. undefined render_bolts, captive_nut_inset, rounded_square, num_front_tabs, etc.

I think a big part of this stems from using "include" rather than "use" in a lot of places. "use" should generally be preferred when possible, rather than the C-preprocessor-like raw file inclusion behavior of "include". "use" prevents undefined variables/functions/modules in a given file from being implicitly satisfied by the file that is using the file and keeps variables from leaking publicly outside of a file; this is not true of "include".

As a basic rule, each individual .scad file should generate no warnings when loaded individually, and additionally, all modules and functions should generate no warnings when used from the same the file they're defined within (sadly this is harder to check/enforce due to the lazy evaluation of modules).

[lastcoolnameleft]

@scottbez1 I fixed the issues you mentioned. Thanks for pointing those out as I was focusing primarily on the main splitflap.scad instead of testing each individual file. I went through and tested each scad file to make sure none of them gave any errors/warnings when previewing.

The merge conflict I encountered from #142 is also fixed in this PR.

Unless there's any other errors/issues I missed, this PR is ready for merge.

[lastcoolnameleft]

@scottbez1 Any thoughts on this? If you're interested in merging this, I'd prefer to do it sooner to prevent merge conflicts as it will get more difficult later on.

[scottbez1]

Thanks so much for the updates, will try to take a look this weekend! From a glance I noticed there are still a number of includes - do you know if all of those have to be include or can some now be changed to use? Eventually I think I'd really like to eliminate include entirely except for a few constant/variable-only files, so any progress toward that is helpful.

[lastcoolnameleft]

I don't know enough about OpenSCAD to know for sure. I'll change all of them over and let you know. That said, I just got my 2nd covid shot, so I might be out of it for the next few days but will get back to it when I can.

[lastcoolnameleft]

@scottbez1 I'm not deep on OpenSCAD, but I think that the includes are necessary based on my initial attempt to remove.

In 3d/pcb.scad, I replaced use <parts/hardware.scad>; with include and tried to render it, but got a compile error:
WARNING: Ignoring unknown variable 'm4_hole_diameter', in file pcb.scad, line 41.

According to https://en.wikibooks.org/wiki/OpenSCAD_User_Manual/Include_Statement use imports modules and functions, but it doesn't say anything about variables.

How would you like to proceed? One option is to put ALL variables into a separate file and only include those and then use anything with a function/module in it. I'd want to think about how to organize that, but I think it's doable.

[Limb]

How would you like to proceed? One option is to put ALL variables into a separate file and only include those and then use anything with a function/module in it. I'd want to think about how to organize that, but I think it's doable.

I would think a master variable file is going against the idea of making the files modular. I would suggest putting the m4 definitions back into m4_dimensions.scad and include them where necessary. The same would apply to other variables, they should be put into an appropriately named file and included where used.

[lastcoolnameleft]

I would suggest putting the m4 definitions back into m4_dimensions.scad and include them where necessary. The same would apply to other variables, they should be put into an appropriately named file and included where used.

This works for me. If we do this, then I would prefer that we be consistent and have a _dimensions.scad or whatever we call it for each of the necessary files. This would increase the number of files, but definitely made it more modular.

If we go down this route, would we want the _dimensions.scad file to live in the same directory, or have a specific dimensions directory?

[scottbez1]

Sorry to leave this hanging, but I'm going to close it for now given some of the other restructuring that has occurred since this was opened, which is now conflicting. I think for a large scale refactor effort to be successful in the future it will probably be best to outline the overall plan (what are the major components, where do configurations live, what does the final component hierarchy look like) and then work on implementing it incrementally over a series of smaller PRs.