kennetek / gridfinity-rebuilt-openscad

A ground-up rebuild of the stock gridfinity bins in OpenSCAD
Other
1.13k stars 164 forks source link

Intended usage as library #157

Open smkent opened 5 months ago

smkent commented 5 months ago

Hi, I'm continuing to find this project super useful!

In using gridfinity-rebuilt-openscad in some of my own projects, I've submitted https://github.com/kennetek/gridfinity-rebuilt-openscad/pull/155 and had/found a few thoughts about using this project as a library. I've collected those here.

Comments from other issues

From https://github.com/kennetek/gridfinity-rebuilt-openscad/issues/125:

@adrianVmariano says:

If global variable parameter passing is the strategy of choice, it would be better to use $ variables so that they can be set from outside. But generally I'd favor an API that does not use global variable parameter passing, but instead allows parts to be created by calls into the library with regular parameters. (If global $ variables are needed internally this can be hidden from the user.)

@Ruudjhuu says:

I do fully agree with the global variables. I tried multiple times to get rid of it, but they are a big pain in the ass. Any pull request regarding improvements here are welcome. (also due to no proper tests (and this is very difficult in openscad), I am scared to touch some parts of the code).

From https://github.com/kennetek/gridfinity-rebuilt-openscad/issues/114:

@kennetek says:

Small slightly-related tangent: The variables inside some of the files that use the $ symbol were from refactoring the code so that everything would be contained in modules (if I remember correctly). I did it in that slightly-bad way because I wanted it done quickly. I don't think that is the proper way to do things, those variables should probably be passed as arguments. Although, that may cause the argument count for many modules to increase dramatically. (Edit: I see that you already plan to remove them)

Path forward

IMO exposing toplevel functions, which is largely what gridfinity-rebuilt-openscad already does, is the right way to go. I've been exposing components from my own OpenSCAD projects this way, such as in my modular hose, rugged box, and Gridfinity rugged box which uses gridfinity-rebuilt-openscad.

Questions:

Ruudjhuu commented 5 months ago

I saw your gridfinity rugged box and am impressed. I will give that a go when I'm done with my backlog of creating bins.

adrianVmariano commented 5 months ago

In my work on the BOSL2 library we have functions that are designated as the API and we have "hidden" functions with are prefixed by '_' and not intended for direct user access. They aren't hidden by the language, but they are designed as hidden and not documented. Functions the users are supposed to use are documented. If you really want to hide stuff it's possible by "use"ing the stuff you want hidden from a separate file, but we found "use" to have serious performance issues so we use "include" instead, despite the namespace clutter that results.

There is a place for $ variables, but I don't think "general parameter" is that place. Interesting things can be achieved by using them to pass information between modules, but when something is a direct parameter, it should just be an argument to the function or module. The role of $ variables, by the way, is when a module computes a value from its arguments and needs to make it available to its children. In BOSL2 $ variables are used to pass geometry information from a parent object to a child object so that the child object can position itself relative to the parent. But direct passing of parameters should be through direct parameters. It might be appropriate for gridfinity to make properties of the object available to children.

While OpenSCAD has many limitations, I do not understand how it presents a particular challenge for doing things the "right way" in this case, with parameters as arguments rather than globals. What do you see as the problem here?

There is a simple answer to supporting the GUI and code: allow both. Check if the user gave a string and if so, change it to the number using a search() call.

adrianVmariano commented 5 months ago

Actually supporting strings should probably be primary (easier to read the code) and a numerical value converted to a string by an array index. Basically you can start the code with something like:

table=["none","big","small",...];
parm = is_string(parm) ? parm : table[parm];

Note that it looks like a reassignment of parm, but actually the parm on the RHS is the module parameter and the one on the LHS is a different variable in a new scope.

adrianVmariano commented 5 months ago

Another thought about the GUI/Customizer vs library: I think the right way to do this is to completely separate the customizer code from the library. The customizer code is a separate file from the library that loads the library and calls it. If something needs to happen in the customizer code (like translating arguments) it can be an extra layer there, rather than having the customizer force bad library design.

smkent commented 5 months ago

My rugged box model is implemented in basically the same way @adrianVmariano describes. I wrote that model with usage as a library being a toplevel use case.

In my work on the BOSL2 library we have functions that are designated as the API and we have "hidden" functions with are prefixed by '_' and not intended for direct user access. They aren't hidden by the language, but they are designed as hidden and not documented. Functions the users are supposed to use are documented.

In my model, all of the "public" modules are named rbox_*, and all the "internal" module names start with an underscore (_). Certainly a library consumer could override a private module, but it's (hopefully) more obvious that isn't the intended use.

Python is similar -- there are no such things as private variables/methods. Python convention is to name private class methods/etc. beginning with an underscore. (I've spent most of my professional career in Python so it is a familiar pattern.)

There is a place for $ variables, but I don't think "general parameter" is that place. Interesting things can be achieved by using them to pass information between modules, but when something is a direct parameter, it should just be an argument to the function or module. The role of $ variables, by the way, is when a module computes a value from its arguments and needs to make it available to its children.

Agreed, this is how I use $ variables in my models. Again using my rugged box model as an example, the toplevel rbox configuration module (similar to gridfinityInit in this project) sets a bunch of $ variables internally so as not to repeat a bunch of parameters in the library's "private" modules.

There is a simple answer to supporting the GUI and code: allow both. Check if the user gave a string and if so, change it to the number using a search() call.

Actually supporting strings should probably be primary (easier to read the code) and a numerical value converted to a string by an array index. [...]

This is a good idea, if we want to preserve compatibility for existing consumers who use the integer constants.

The string constant used for an option doesn't have to match its display value either. For example, the Lip_Seal_Type option in my model uses string constants for the values but has friendlier display strings for the customizer.

Another thought about the GUI/Customizer vs library: I think the right way to do this is to completely separate the customizer code from the library. The customizer code is a separate file from the library that loads the library and calls it. If something needs to happen in the customizer code (like translating arguments) it can be an extra layer there, rather than having the customizer force bad library design.

This is where I landed too. My rugged box model's toplevel file (rugged-box.scad) exposes a bunch of customizer options, and then just invokes the main library file configuration and part modules.


The upside is that gridfinity-rebuilt-openscad already follows most of this convention. We could support string constants for the enum values to make things friendlier and update module names to indicate which modules are "public." I think most importantly it's just good to confirm what the intended approach is to ensure consistency as the project moves forward.