BelfrySCAD / BOSL2

The Belfry OpenScad Library, v2.0. An OpenSCAD library of shapes, masks, and manipulators to make working with OpenSCAD easier. BETA
https://github.com/BelfrySCAD/BOSL2/wiki
BSD 2-Clause "Simplified" License
1.01k stars 115 forks source link

Propagate information about partition() part to children() #1480

Closed zouppen closed 1 month ago

zouppen commented 1 month ago

This patch allows to customize the partitioned parts. The part is available in $partition_part variable and is either FIRST or SECOND).

adrianVmariano commented 1 month ago

Probably the values should be 0 and 1 instead of FIRST or SECOND. I also wonder if it should be $idx or $partition_idx. Should we rename some of the other $idx variables to $<module>_idx to keep them from stepping on each other?

zouppen commented 1 month ago

One of the hard things in computing, whether to start from 0 or 1. My initial thought was that if we ever have BOTH (or ALL) then it could be zero, for example in case we'd add an argument like partition(keep=BOTH|FIRST|SECOND).

About the prefixing question: I have no preference since I'm not too familiar with your coding conventions. Of course non-colliding variable names are better than colliding :)

adrianVmariano commented 1 month ago

In a whole bunch of places modules propagate $idx with a zero base to refer to iterates of the module. See for example everything in distributors.scad. So the simplest more clearly consistent behavior would be to do the same. Doing $partition_idx could, like I said, keep things more separated, though probably a collision is unlikely in this case. Is "both" likely? I have no idea. I do not like introducing FIRST and SECOND which seem like needless namespace clutter.

zouppen commented 1 month ago

I also have mixed feelings about them, maybe my history on statically typed languages led me to introduce them. Should we use a boolean instead? I can fix the patch, but I'd like to have your opinion about naming and typing, like:

adrianVmariano commented 1 month ago

Do you think it is likely that someone would want to condition on which partition within the code inside other stuff, like grid() or xcopies() or the like? If this seems very unlikely I think maybe using $idx set to 0/1 would be best because it follows a pattern used in many other modules.

If we think collision is more likely then I like $partition_idx.

zouppen commented 1 month ago

Hmm, maybe then I'd say it's best to go with the current convention. The collision is rather easy to avoid in user code by doing something like $partition_idx = $idx right inside the nested block before xcopies() or the like, if they need the partition index all the way down.

I'll prepare the patch tomorrow. :)

adrianVmariano commented 1 month ago

Yes, exactly. That's why I'd only deviate from that convention if it seems like a common or likely collision situation. So I think going with $idx set to 0/1 makes sense.

revarbat commented 1 month ago

Agreed with using $idx with values of 0 and 1 instead of constants.

zouppen commented 1 month ago

GitHub has lost track of my commit for this pull request, maybe because I've done both rebase and force-with-lease to my branch. I close this and open a new one #1487