Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intended usage as library #157

Open
smkent opened this issue Jan 29, 2024 · 5 comments
Open

Intended usage as library #157

smkent opened this issue Jan 29, 2024 · 5 comments

Comments

@smkent
Copy link
Contributor

smkent commented Jan 29, 2024

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

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

Comments from other issues

From #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 #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:

  • Will this continue to be the intended usage pattern for this project?
  • Is there interest in migrating more options to modules and arguments? For example, my rugged box library has a lot of options so I separated out the basic and advanced options into two different initialization modules (rbox and rbox_size_adjustments). We could set up the Gridfinity standard values in the same way and allow them to be overridden using a module.
  • Can we support string constants instead of integers for the fixed style options? For example, style_tab currently has possible values of 0, through 5. This could be changed to use the string constants full, auto, left, center, right, and none.
@Ruudjhuu
Copy link
Collaborator

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.

  • I do not think exposing top level functions is the correct way. But afaik in openscad there is no alternative and all functions/modules are top level.
  • Regarding global variables. Open scad is a functional language. One of the principles is: "pure functions". This means anything used from outside the function that is not a constant, should be an input argument. Constants are: pi, imaginary unit, Euler's number, etc..
  • Regarding interest in migrating options to modules and arguments. I don't know anymore. I think openscad fails to facilitate doing things the "right way". Your example is a fine solution, but none of the modules are pure, and all variables are in the global scope which gives me goose bumps as a developer, but the alternatives are also questionable
  • Support string constants: If the GUI is used, this is the way to use some sort of messed up enum. For the GUI it are string constants (drop down list with options). However, within the code it is just an int accessible only by its number. Afaik there is no sane enum support in OpenScad. I think we need to make a traid off: usability in the GUI or usability in the code. This project chose GUI.

@adrianVmariano
Copy link

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
Copy link

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
Copy link

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
Copy link
Contributor Author

smkent commented Jan 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants