jscad / csg.js

DEPRECATED: CSG Library for JSCAD (See the link below)
https://github.com/jscad/OpenJSCAD.org/tree/master/packages/modeling
MIT License
217 stars 56 forks source link

Remove polymorphic math operators #152

Closed pentacular closed 5 years ago

pentacular commented 5 years ago

I came across these when figuring out why the circle generator was broken -- it had the arguments to scale backward.

The polymorphic code makes the math library functions much less readable, and adds significant complexity to the code.

It also seems to violate the api requirement for useful curryability.

It doesn't seem to be used, and any uses are trivially refactorable.

So I propose removing it.

Let's add additional (and distinct) operators like this when profiling tells us that it's worthwhile.

z3dev commented 5 years ago

It would definitely be more natural to add an optional parameter to the end. That's my opinion.

const vec1 = vec2.abs(prev)
const vec1 = vec2.abs(prev, vec1)

Also, I'm finding the ability to update in-place is useful. And we haven't even started to appy optimzations.

pentacular commented 5 years ago

I think it's fine to have destructive versions of the function. What I'd like to avoid is having the two versions overloaded with dynamic dispatch.

How about adding a To on the end to indicate that they destructively modify the last argument.

e.g., addTo(a, b, out)

z3dev commented 5 years ago

Better. This is getting better. I would prefer to keep in-place updates.

How about ...

absInto(out, vector)
addInto(out, vectorA, vectorB)
...

Which reads almost like English. Well... kind of...

And very apparent as to what the logic is doing inside code blocks.

kaosat-dev commented 5 years ago

sorry but that is kindof not nice api at all , can we please keep the maths as it is for now ? I agree it is not perfect, but it works, and accomodates for mutations & not mutations without becoming unreadable or overly complex and two versions of each operator is just ... not nice

pentacular commented 5 years ago

sorry but that is kindof not nice api at all , can we please keep the maths as it is for now ? I agree it is not perfect, but it works, and accomodates for mutations & not mutations without becoming unreadable or overly complex

and two versions of each operator is just ... not nice

What you currently have is two versions of almost every operator, with two different call signatures.

add(a, b) add(out, a, b)

They're just hidden inside a function which is doing dynamic dispatch on every call.

And given that almost every call is of the add(a, b) form, that dispatch is pure overhead for the common case.

I really can't find any scenario in which this is a good thing.

At least if you turn that into: add(a, b) addInto(a, b, out)

then the dispatch will disappear from the common case, and the function signatures will be coherent.

Regarding readability.

Currently you have add = (...params) => { ... } which requires reading the function body to figure out what the arguments are. And then on the call side, you need to count the arguments to see which it's doing.

This makes it hard to even determine how many calls in the code-base are made to add(a, b, c) as opposed to add(a, b).

I guess you could write a code walker to do it, otherwise you're down to playing games with regex.