Reorganization of Geometries

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 Geometries #167

Closed z3dev closed 5 years ago

z3dev commented 5 years ago

WORK IN PROGRESS DO NOT MERGE

The goal of this pull request is to complete the basic geometries. Each should be complete, 100% tested, reviewed, and ready for use in other functionality.

Geometries:

geom2
- [x] code complete
- [x] tests complete, 100% coverage
- [x] reviewed
geom3
- [x] code complete
- [x] tests complete, 100% coverage
- [ ] reveiwed
path2
- [x] code complete
- [x] tests complete, 100% coverage
- [ ] reveiwed
poly3
- [x] code complete
- [ ] tests complete
- [ ] reviewed

z3dev commented 5 years ago

@kaosat-dev please review the code for geom2, geom3, path2.

i need to add some tests to obtain 100% coverage.

are you going to make the changes to poly3 on this branch? (moving to array of three points and plane)

z3dev commented 5 years ago

i need to add some tests to obtain 100% coverage.

The test suites for geom2, geom3, and path2 are complete. All have 100% coverage now. :)

kaosat-dev commented 5 years ago

I will check things out tomorrow, thanks @z3dev ! No, any changes I will make will be after this, in another branch :) (keeping things simple & manageable)

z3dev commented 5 years ago

Some things to think about...

Consistency... The geometries should be small and concise. Anything else, like geom2.toOutlinePaths, should go into the operations. There's some incosistency with naming as well, i.e. reverse, flip, etc.
Parameter checking... how much should be performed? see geom2.fromPoints()
Canonicalize... I think we should drop the canonicalize of vec2, and use full float values. It's my belief that the precision issues are due to the reuse of planes, not the point values. Planes should never be reused.

z3dev commented 5 years ago

@kaosat-dev Please review geom3 and path2, as these have been updated.

renamed canonicalize to applyTransforms
removed isCanonicalized as the transforms can be used to determine state
updated all logic and tests
added further transform tests, following a typical user scenario

z3dev commented 5 years ago

@kaosat-dev Please review geom3 and path2, as these have been updated.

@kaosat-dev Can you review and merge this pull request?

If any changes are required then let me know. Cheers

kaosat-dev commented 5 years ago

this all looks amazing ! thanks @z3dev going to merge now !