Isolating/Refactoring the math library

jeromew commented 7 years ago

This is to open a discussion and try to help in the work that has started around the math/geometry library.

Maybe at one point it will be interesting to switch the library to one of the existing js math libraries. Nevertheless, it seems that there are still some unclear (to me at least) dependencies between the math part and the library and the rest of the csg library.

After a first look, something unclear appeared to me : primitive2d.js is a big user of math primitives to construct 2d objects. It builds the objects as "points", "path", "cag"

I tend to agree that when building a primitive, I would like to have access to all the math, cag, csg API.

but one unclear dependency appears in this list

return CAG.fromPoints(points) -> ok
return e2.innerToCAG() -> ?
..

The innerToCAG is implemented in https://github.com/jscad/csg.js/blob/master/src/math/Path2.js#L160 and could probably be replaced with a function to get the points + an isClosed function to check if the path is closed.

On the same vein, expandToCAG and rectangularExtrude probably should'nt be in path. A function "getSides" may help to remove expandToCAG but I don't know what to do of rectangularExtrude. This seems pretty high-level.

So maybe the "primitive" level could include both the building of objects and the transformation of objects like extrusion (or maybe there are already somewhere else)

what do you think ? I could try and send a PR on this.

jeromew commented 7 years ago

for reference https://github.com/jscad/csg.js/issues/24 deals with duck-typing in the math library.

kaosat-dev commented 7 years ago

Also not 100% sure on all the relationships, a lot of things could be seperated out between 'core' functionality and stuff build on top of it. I had started mapping out some of these things , this could help perhaps : https://gist.github.com/kaosat-dev/dfad0f50aa4e844e0958d7c7cf308799

z3dev commented 7 years ago

I'm all for trying to define the "core" functionality. Everything else, like rectangular extrude, expand, etc really should be 'functions' that are not tied to an the object prototype.

Mark will Iike this...

jeromew commented 7 years ago

@z3dev i don't understand your message. You mean for example that "rectangular extrude" should simply be a function that combine other operations ?

it is true that primitives are currently not bundled inside an object scope. It is not yet clear to me where they should belong, but if we want to abstract a math library that has no knowledge of csg.js these cannot be in the low level math/geometry objects

Beeing new to the project I did not understand your "..." about Mark.

jeromew commented 7 years ago

I looked at Path2.innerToCAG() which should I think be moved to CAG.fromPath2(path) to remove the dependency of Path2 on CAG. Now I am left with the choice of putting it in CAG.js or in CAGFactories.js. It seems to me that CAGFactories would be the best ?

now it seems there are also factories inside CAG.js (fromPoints, fromSides, ..)

should I move everything that looks as a factory (CAG.fromXXX) to CAGFactories ?

now that I open the files I understand that the code footprint is huge. Not sure I embrace why yet but there are a lot of things going on..

kaosat-dev commented 7 years ago

@jeromew Mark would be me haha :) I am a strong proponent of functional oriented code , and a lot of CSG.js is 'false' oop that is hiding some nice functional code.

About Path2.innerToCAG() agreed:

moving it to CAGFactories indeed seems to be the best
although perhaps even CAGFactories is too 'big', because each factory function has its own 'require'ments : users might want to use a given factory function without having the dependency of all others

z3dev commented 7 years ago

@jeromew I wouldn't worry too much about unwinding the dependencies. @kaosat-dev spent some major effort to get the library to this state. There's a lot more that can be done.

It seems to me that the from*() functions should be part of the CAGFactories as well.

What about CAG.fromCompactBinary()? Where did that go? I think this should be part of the CAGFactories as well.

jeromew commented 7 years ago

I sent a PR regarding fromPath2 - https://github.com/jscad/csg.js/pull/49 tell me what you think

jeromew commented 7 years ago

Also @kaosat-dev regarding

although perhaps even CAGFactories is too 'big'

I agree that CAGFactories/CAGMakers will become bloated if we put all fromXXX inside it. Nevertheless, their function is always to make a CAGs. primitives2d.js are also CAGFactories in some way. If you want to split the code in several one-function-files it will be easy to do but I suggest we do it later. At some point it will be necessary to be able to accept CAGMakers directly from npm : someone could want to publish specific models like in http://microsoft.github.io/maker.js/. But for this jscad will need some plugin system.

jeromew commented 7 years ago

Also, let me add that jscad is a "many-modules" project. Maybe it could benefit from something like https://lernajs.io/ . Lerna was developed for babel initially because managing several interdependant modules rapidly becomes a versioning nightmare. pug.js also uses lerna.

kaosat-dev commented 7 years ago

@jeromew yeah one step at a time ! About Lerna.js : actually that is something I have experimented on when toying with refactoring options of OpenJSCAD.org : one of the split out repositories actually follows a lot of the Lerna conventions and is a monorepos (https://github.com/jscad/io) with a lot of sub (npm) packages in the 'packages' folder. There are a few issues with Lerna at this stage though: (might have changed)

lack of support for scoped packages
some weird issues with corner cases (I stumbled upon one when testing)
perhaps too complex ? (they were working on simplifying)
csg.js for example might need to stay independent as it has a wider scope ?

100% worth keeping an eye on though

z3dev commented 6 years ago

@jeromew The V2 of CSG is well underway now. It would be good to get your thoughts on the progress.

And thanks for the information on Lerna. It seems that @kaosat-dev actually moved JSCAD to it.

jeromew commented 6 years ago

@z3dev sorry I have been mostly AFK for some time.

overall jscad's code has many many parts and i know after scanning through it a few months ago that the refactoring is far from easy. The csg.js V2 branch has already many structure improvements and tests ! nice work !

I looked a bit at vec2, for example https://github.com/jscad/csg.js/blob/V2/src/core/math/vec2/add.js

is it really necessary to keep both "in-place" (with a prepared receiving vector) and "create" (where the receiving vector is created just in time) modes ? When I look at gl-vec2, they only have the "in-place" operation and avoid a systematic if (params.length === test in all operations. I would think (not tested) that keeping both modes has a real perf cost in a math-heavy library like csg.

z3dev commented 4 years ago

@jeromew Have you played with V2 lately? I think that the API is nearly complete, data structures defined, and dependencies corrected. The whole library is clean and understandable now.

We would like your feedback if possible. (Please open new issues as part of OpenJSCAD.org)

jeromew commented 4 years ago

@z3dec I received your ping. Huge work on the refactoring it will be many times mode clear ! my main feedback is about the fact that many math APIs have 2 signatures : if the structure is not already created, it will be created on the fly. for example look at https://github.com/jscad/csg.js/blob/V2/src/math/vec3/max.js the systematic if (params.length === 2) {

I suspect that in the long term this could be a performance hit for the library + now that you cleared all the dependencies, I wonder if csg.js should not try to direcly use gl-matrix instead of basically implementing the same operations (+ some operations that gl-matrix does not have).

I wonder if three.js is not taking the same path. If you look at https://github.com/mrdoob/three.js/blob/dev/src/math/Line3.js, you can see that all the code paths where arguments are not set now trigger warnings - https://github.com/mrdoob/three.js/blob/dev/src/math/Line3.js#L86

You can see the three.js reasoning in this ticket https://github.com/mrdoob/three.js/issues/12231 (mainly https://github.com/mrdoob/three.js/issues/12231#issuecomment-331605172) and I think it is the same for csg.js

I hope this can help you

z3dev commented 4 years ago

@jeromew Thanks for the feedback.

Yeah. Those extra Boolean comparisons may be expensive. But we will have to compare the performance hits. There may be performance hits on using ARGS as well.

I’ll keep this open until the performance comparison can be completed.

jeromew commented 4 years ago

another link regarding the discussion about this in gl-matrix when they were planning for v2 (2012) - https://github.com/toji/gl-matrix/issues/41

z3dev commented 3 years ago

@jeromew do you mind if I close this issue here?

The math library is now separated, and the API for V2 has been completed. See https://www.jscad.xyz/docs/

I have also added a task for reworking the API in V2.1.

jeromew commented 3 years ago

@z3dev no you are right this issue can be closed

jscad / csg.js

Isolating/Refactoring the math library #48