search

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

Reorganization of Booleans #170

Closed z3dev closed 5 years ago

z3dev commented 5 years ago

READY FOR MERGING

This pull request adds the boolean functions as a seperate module. The goals are:

This pull request adds the measurements module as well due to dependency on those functions. The goals are:

Booleans:

intersect

subtract

union

Measurements:

measureArea

measureBounds

measureVolume

z3dev commented 5 years ago

@kaosat-dev please review and provide feedback.

There are two modules in this pull request; measures and booleans. So, the first question is... Should these be part of the 'operations' module, or live side by side with 'math' and 'primitives'?

The two modules are structured differently. The measures have only a single file for each exposed API, and the geometry specific functions are private. The booleans have multiple files for both exposed API and geometry specific functions.

Which way to package these? What should we expose via the index?

z3dev commented 5 years ago

There are two modules in this pull request; measures and booleans. So, the first question is... Should these be part of the 'operations' module, or live side by side with 'math' and 'primitives'?

@kaosat-dev do you have any thoughts about this?

kaosat-dev commented 5 years ago

I think it should be seperate modules ? Then again, measurements are used a lot elsewhere, but that is no issue I think :)

kaosat-dev commented 5 years ago

Wait I missed a part of your question : what do you mean by "and the geometry specific functions are private" ?

z3dev commented 5 years ago

Wait I missed a part of your question : what do you mean by "and the geometry specific functions are private" ?

For example.... measureArea() is exported in measureArea.js. There are non-exported functions to calculate area for geom2, geom3, and path2. Those functions could also be exported, as well as broken out into specific files.

I added 'index.js' to both booleans and measures modules. @kaosat-dev please review the exports. should additional functions be exported?

z3dev commented 5 years ago

Next up... the 'utility' functions being used by the booleans functions.

@kaosat-dev any thoughts?

z3dev commented 5 years ago

Logic question...

union() calls mayOverlap() in order to use the faster unionForNonIntersecting() function.

why not use the same logic for intersect() and subtract()? seems reasonable to me.

kaosat-dev commented 5 years ago

For example.... measureArea() is exported in measureArea.js. There are non-exported functions to calculate area for geom2, geom3, and path2. Those functions could also be exported, as well as broken out into specific files.

All things should be specific to each data/ geom type right ?

kaosat-dev commented 5 years ago

fromFakePolygons ; just leave here? rename to from3DWalls?

I have to take a closer look, I think there is already something named similarly to from3dWalls ?

kaosat-dev commented 5 years ago

For all the others: yes I would leave as such and leave them there for now

kaosat-dev commented 5 years ago

to3DWalls I have some doubts about the naming of that one, but for now I think it is clear enough

kaosat-dev commented 5 years ago

why not use the same logic for intersect() and subtract()? seems reasonable to me.

That is VERY smart thing to do !

z3dev commented 5 years ago

something named similarly to from3dWalls

fromFakePolygons() is actually the opposite of to3DWalls(). So, I thought we could make these into a pair of utility functions.

it seeems we were thinking the same a while back, as there is an old file called...

z3dev commented 5 years ago

UG!

OrthoNormalBasis is being used by reTesselateCoplanarPolygons(). Should I remove this now? I think we can follow the suggestion in (closed) PR #160

z3dev commented 5 years ago

why not use the same logic for intersect() and subtract()? seems reasonable to me.

That is VERY smart thing to do !

DONE. :)

z3dev commented 5 years ago

@kaosat-dev please perform a code review. all code is complete, as well as initial test suites.

z3dev commented 5 years ago

@kaosat-dev This is ready for merging. If you don't have any further concerns or comments then please merge.

And like always... Thanks for the excellent reviews!

kaosat-dev commented 5 years ago

thanks for the last batch of changes, looks awesome ! Thanks for the hard work as always ! ;)