RaulBejarano / Ultimate-Toroidal-Propeller-Generator

The Ultimate Toroidal Propeller Generator is an open source project that provides a way to generate STL files of toroidal drone propellers.
GNU General Public License v3.0
115 stars 18 forks source link

increase rendering quality and speed and avoid non-manifold objects #11

Closed schiele closed 1 year ago

schiele commented 1 year ago

This applies some OpenSCAD good practices that increase rendering quality and speed and avoid non-manifold objects.

RaulBejarano commented 1 year ago

Hi @schiele! First of all, thank you so much for contributing, this is my first released open source project and it's exciting to have PRs. I will review your changes and test them as soon as possible.

schiele commented 1 year ago

Okay, I understand why you're using the epsilon variable, but I don't quite get why you declare it outside of the module. If someone imports the file using 'include' instead of 'use', they could potentially modify the value of epsilon. Wouldn't it be better to declare it inside if it's meant to be a 'constant'?

In the end, this boils down to a matter of taste. The reason I did it like this is:

  1. I would not consider someone using include instead of use so much of an issue since it is common practice to use libraries with use unless this is a library specifically designed to provide constants. It is common practice among OpenSCAD libraries even to add examples inside the library code itself, meaning whenever someone used include to get them they would get all the sample objects.
  2. If you put it inside a module it works only for this module. Over time you might add other modules needing the same constant, which would mean you would need to duplicate the definition or hide it inside a function definition.

So in the end you need to decide what is your preference, whether you want to keep it I did it, move it to the module, or maybe add a unique prefix to make the name less likely to conflict with other code (but therefore make your code a bit more ugly).

RaulBejarano commented 1 year ago

Thanks for the explanation, I will keep your code.