gsohler / openscad

OpenSCAD - The Programmers Solid 3D CAD Modeller
https://www.openscad.org
Other
26 stars 7 forks source link

Changing operators to match set() #12

Closed Matthieu-LAURENT39 closed 1 year ago

Matthieu-LAURENT39 commented 1 year ago

Reasoning

The + operator for union() and the - operator for difference() feel intuitive, but * for intersection() doesn't seem to make too much sense, we're not multiplying objects together (instead, maybe it could be used to scale objects, but that's a discussion for another day).

I propose we instead use the same operator as the set intersection: & This would not only be more intuitive (it's a binary and), but also make a lot of sense.

And overall, it would a lot more sense to move to using all set operators. Python has a dedicated union operator (|), and it feels weird to instead use the + operator. The difference operator is, as the name suggests, still -. This is consistent with set()'s behaviour

Suggested changes

While the Zen of Python states "There should be one-- and preferably only one --obvious way to do it.", it may be a bad idea to start depreciating such an important feature. One thing is sure, if we're going to remove the + operator, it's now or never. Once the PR is merged, we won't get the chance.

On a side note, i'm working on a PR to add such changes, but i'm waiting on your feedback about the deprecation @gsohler

gsohler commented 1 year ago

Hi Laurent, I understand your reasoning. ! For boolean ops there are two world of operators: algebraic and logical

Union 0+0 = 0 0+1 = 1 1+0 = 1 1+1 >= 1

Intersection 00 = 0 01 = 0 10 = 0 11 >= 1

Difference 0-0 = 0 0-1 <= 0 1-0 = 1 1-1 >= 0

Issue is that &~ for difference is not very intuitive (And openscad does not have pure inverse operator). So we could keep at least - and (and ultimately all algebraic ops)

For me both sets are valid and logical. I doubt that we will run out of operators at any time. We could document & and | , - and implement both sets

in fact * is already used for scaling(differentatin is done by argument type)

can you elaborate set() operator ?

On Fri, Sep 8, 2023 at 5:18 PM Matthieu LAURENT @.***> wrote:

Reasoning

The + operator for union() and the - operator for difference() feel intuitive, but * for intersection() doesn't seem to make too much sense, we're not multiplying objects together (instead, maybe it could be used to scale objects, but that's a discussion for another day).

I propose we instead use the same operator as the set intersection: & This would not only be more intuitive (it's a binary and), but also make a lot of sense.

And overall, it would a lot more sense to move to using all set operators. Python has a dedicated union operator (|), and it feels weird to instead use the + operator. The difference operator is, as the name suggests, still -. This is consistent with set()'s behaviour Suggested changes

  • Deprecate the * operator (or even better, remove it)
  • Add the & intersection operator
  • Add the | union operator
  • Potentially deprecate the + operator

While the Zen of Python states "There should be one-- and preferably only one --obvious way to do it.", it may be a bad idea to start depreciating such an important feature. One thing is sure, if we're going to remove the + operator, it's now or never. Once the PR is merged, we won't get the chance.

On a side note, i'm working on a PR to add such changes, but i'm waiting on your feedback about the deprecation @gsohler https://github.com/gsohler

— Reply to this email directly, view it on GitHub https://github.com/gsohler/openscad/issues/12, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCO4MXSUIR7WDJFTPJB5OLXZMZL5ANCNFSM6AAAAAA4QRGW3I . You are receiving this because you were mentioned.Message ID: @.***>

Matthieu-LAURENT39 commented 1 year ago

in fact * is already used for scaling(differentatin is done by argument type)

The fact * does 2 completely different things (scaling and intersection) seems even more confusing :sweat_smile: Also, as i said, we should use - for difference (and not &~), that's what python uses for set differences

can you elaborate set() operator ?

As for set() operators, here's the full table:

Operator Symbol Example
Union \| {1, 2} \| {2, 3} returns {1, 2, 3}
Intersection & {1, 2} & {2, 3} returns {2}
Difference - {1, 2, 3} - {2} returns {1, 3}
Symmetric Difference ^ {1, 2} ^ {2, 3} returns {1, 3}

For me both sets are valid and logical. I doubt that we will run out of operators at any time.

It's not really about running out of operators, and more that there should be one operator to do one thing (once again the zen of python: "There should be one-- and preferably only one --obvious way to do it."). And since we're doing unions, intersections, and differences, it seems more intuitive to use the set operators.

gsohler commented 1 year ago

Another thing, is that + - and * are much more accessible on a keyboard and more easier to understand for people who did not go to school for more than 8 years in their life. But let's be syntactically correct and perform the PR :)

BTW: we should never actually offer the ^ operator as we will extensively check the boolean engine . result will be prone to manifold errors ;)

On Fri, Sep 8, 2023 at 8:49 PM Matthieu LAURENT @.***> wrote:

in fact * is already used for scaling(differentatin is done by argument type)

The fact * does 2 completely different things (scaling and intersection) seems even more confusing 😅 Also, as i said, we should use - for difference (and not &~), that's what python uses for set differences

can you elaborate set() operator ?

As for set() operators, here's the full table: Operator Symbol Example Union Intersection & {1, 2} & {2, 3} returns {2} Difference - {1, 2, 3} - {2} returns {1, 3} Symmetric Difference ^ {1, 2} ^ {2, 3} returns {1, 3}

For me both sets are valid and logical. I doubt that we will run out of operators at any time.

It's not really about running out of operators, and more that there should be one operator to do one thing (once again the zen of python: "There should be one-- and preferably only one --obvious way to do it."). And since we're doing unions, intersections, and differences, it seems more intuitive to use the set operators.

— Reply to this email directly, view it on GitHub https://github.com/gsohler/openscad/issues/12#issuecomment-1712091554, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCO4MSBWQK2KOFWXXSDYGTXZNSENANCNFSM6AAAAAA4QRGW3I . You are receiving this because you were mentioned.Message ID: @.***>

Matthieu-LAURENT39 commented 1 year ago

I'll probably have the PR done tomorrow.

BTW: we should never actually offer the ^ operator as we will extensively check the boolean engine . result will be prone to manifold errors ;)

Yeah, i wasn't suggesting using it, it was just to mention all the set operators for the sake of completion :+1:

Matthieu-LAURENT39 commented 1 year ago

On another note, for the PR, should i deprecate *, and/or +?

gusohler commented 1 year ago

Yes Lets depreciate the + for Union..this will mainly Help me.iT should BE Used for translate instead. And ITS absolutely OK to Scale before translate. Chöosing an Operator for rotate would also bei sexy .Solid = 3d vector could BE Used to Scale to know Dimensionen.

Matthieu LAURENT @.***> schrieb am Fr., 8. Sep. 2023, 23:07:

On another note, for the PR, should i deprecate *, and/or +?

— Reply to this email directly, view it on GitHub https://github.com/gsohler/openscad/issues/12#issuecomment-1712222883, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADLLHIE5KZXRKDB4CZA4XVTXZOCIVANCNFSM6AAAAAA4QRGW3I . You are receiving this because you are subscribed to this thread.Message ID: @.***>

Matthieu-LAURENT39 commented 1 year ago

not everything needs an operator, it's nice to have them for things where you can fairly easily understand what they're doing (like scaling), but rotate doesn't have an operator that would be "logical". And rotate isn't so common that it's impractical that it doesn't have it's own operator, i think we can do without

gusohler commented 1 year ago

OK

BTW I am almost done with replacing the arrays of union etc to plain arguments and it works. make sure that you also get the crests clean for your PR basically ctest must pass but test first ctest -R primitives ctest -R abuse

cheers

On Sat, Sep 9, 2023 at 12:25 PM Matthieu LAURENT @.***> wrote:

not everything needs an operator, it's nice to have them for things where you can fairly easily understand what they're doing (like scaling), but rotate doesn't have an operator that would be "logical". And rotate isn't so common that it's impractical that it doesn't have it's own operator, i think we can do without

— Reply to this email directly, view it on GitHub https://github.com/gsohler/openscad/issues/12#issuecomment-1712478169, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADLLHIGSJJSYGC6BLIJOGYLXZQ7YNANCNFSM6AAAAAA4QRGW3I . You are receiving this because you commented.Message ID: @.***>

Matthieu-LAURENT39 commented 1 year ago

I'll wait for you to merge first then, to avoid merge conflicts

gusohler commented 1 year ago

I have merged Yesterday already. Dont think there will merge conflicts. You can now Grab latest python-pr3

Matthieu LAURENT @.***> schrieb am Sa., 9. Sep. 2023, 23:31:

I'll wait for you to merge first then, to avoid merge conflicts

— Reply to this email directly, view it on GitHub https://github.com/gsohler/openscad/issues/12#issuecomment-1712645485, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADLLHIBJXNNZVO4MSLWT4S3XZTN4RANCNFSM6AAAAAA4QRGW3I . You are receiving this because you commented.Message ID: @.***>

gsohler commented 1 year ago

i can help out if you are stuck with anything. if you conflict with the ctest, this is not an issue, i can also fix it.

gsohler commented 1 year ago

operarators are change now to &, |, -

did you check latest tutorial ?