Reorganization of Primitives

z3dev commented 5 years ago

This pull request adds the 2D and 3D primitives as a separate module. The goals are:

Convert to V2 math
Standardize API across all primitives; parameters, names, etc.
Add checks of passed parameters, and toxic combinations
Generate 2D and 3D geometries

Major Changes:

CHANGED startAngle and endAngle options to RADIAN values
CHANGED cylinder() cylinderElliptic() roundedCylinder() to use 'height' option
RENAMED roundedCube() to roundedCuboid()
NEW ellipsoid() primitive
NEW star() primitive

Primitives:

arc

[X] code complete
[X] tests complete, 93.18% coverage
[X] review complete

circle (alias of ellipse, see ellipse.js)

[X] code complete
[X] tests complete

cube (alias of cuboid, see cuboid.js)

[X] code complete
[X] tests complete

cuboid

[X] code complete
[X] tests complete, 83.33% coverage
[X] review complete

cylinder (alias of cylinderElliptic, see cylinderElliptic.js)

[X] code complete
[X] tests complete

cylinderElliptic

[X] code complete
[X] tests complete, 92.06% coverage
[X] review complete

ellipse

[X] code complete
[X] tests complete, 82.76% coverage
[X] review complete

ellipsoid

[X] code complete
[X] tests complete, 92.06% coverage
[X] review complete

geodesicSphere

[X] code complete
[X] tests complete, 98.53% coverage
[X] review complete

line

[X] code complete
[X] tests complete, 83.33% coverage
[X] review complete

polygon

[X] code complete
[X] tests complete, 86.84% coverage
[X] review complete

polyhedron

[X] code complete
[X] tests complete, 84.21% coverage
[X] review complete

rectangle

[X] code complete
[X] tests complete, 81.82% coverage
[X] review complete

roundedCuboid

[X] code complete
[X] tests complete, 98.20% coverage
[X] review complete

roundedCylinder.js

[X] code complete
[X] tests complete, 94.2% coverage
[X] review complete Closed issue #21 as well.

roundedRectangle

[X] code complete
[X] tests complete, 93.75% coverage
[X] review complete

sphere (alias of ellipsoid, see ellipsoid.js)

[X] code complete
[X] tests complete

square (alias of rectangle, see rectangle.js)

[X] code complete
[X] tests complete

star

[X] code complete
[X] tests complete, 91.89% coverage
[X] review complete

torus

[X] code complete
[X] tests complete, 2 skipped tests
[X] review complete
NOTE: DIsabled due to dependency on extrudeRotate. Should this be part of extrusions?

z3dev commented 5 years ago

It's time for another long discussion. I added notes to those primitives with issues.

It's my feeliing that primitives should be just that, and not rely upon other operations to produce geometry. For example 'torus' seems like a primitive but relies upon 'extrudeRotate'. This is a special compound shape, and may be better packaged as part of 'extrusions'.

z3dev commented 5 years ago

@kaosat-dev You will like this... it seems that the logic inside sphere() was actually for ellipsoid(), but had the X, Y, Z radius set to the same value. I changed things, exposing ellipsoid as a primitive.

https://en.wikipedia.org/wiki/Ellipsoid

kaosat-dev commented 5 years ago

very cool @z3dev ! I am currently reviewing, thanks for the awesome work, will post back in a bit

z3dev commented 5 years ago

@kaosat-dev What to do with polygon? The API is still very SCAD-like, and very confusing. I'm not sure why the complex logic for lists-of-lists was required.

kaosat-dev commented 5 years ago

@z3dev can you please explain a bit more about what you mean by list of lists ?

kaosat-dev commented 5 years ago

@kaosat-dev You will like this... it seems that the logic inside sphere() was actually for ellipsoid(), but had the X, Y, Z radius set to the same value. I changed things, exposing ellipsoid as a primitive.

https://en.wikipedia.org/wiki/Ellipsoid

Haha that is good indeed !

z3dev commented 5 years ago

@z3dev can you please explain a bit more about what you mean by list of lists ?

polygon() needs some help...

it should follow polyhedron, and ONLY use named options; 'points' and 'paths'
the internal logic (line 56) seems to imply that paths is an array of paths, and each path is an array of indexes. this implies that MANY polygons are combined into ONE 2D geometry. is this intended?

i think this is unsupportable as written. there are many issues with rotation, hole detection, etc.

z3dev commented 5 years ago

polgyon()

it should follow polyhedron, and ONLY use named options; 'points' and 'paths' DONE

z3dev commented 5 years ago

roundedCube() needs to be rewritten as well. There are precision errors if the resolution is adjusted to a sphere that is not planiar with X,Y,Z. For example.

let obj = CSG.roundedCube({radius: [5,10,15], roundradius: 3, resolution: 6});

kaosat-dev commented 5 years ago

@z3dev good job on the naming of polyhedron: for now I would keep it 'as it is ' for the rest and revisit later : that function is used A LOT for all stl & other format imports, so breaking things even further can wait ? what do you think ?

about roundedCube: ah yes, lots of pitfalls ! (I even recall that we had notes about that one ?) also just thought about something : I think it should be roundingSegments or roundSegments for the rounding 'resolution' to make it clearer About the precison errors, set of incompatible parameters: can they be caught and an error be thrown or are there realistically too many issues ?

z3dev commented 5 years ago

polygon() : so breaking things even further can wait ? what do you think ?

Sounds like a good suggestion. Let's run with the latest. There could be a few more changes coming.

z3dev commented 5 years ago

WOW. That was one major pull request!

I'm fine with merging now.

@kaosat-dev This is now done, unless you want further changes.

kaosat-dev commented 5 years ago

This was epic, thanks a ton @z3dev ! WELL DONE ! merging :)

jscad / csg.js

Reorganization of Primitives #169