V2 Reorganization - Path appendArc appendBezier appendPoints

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

V2 Reorganization - Path appendArc appendBezier appendPoints #178

Closed z3dev closed 4 years ago

z3dev commented 5 years ago

This pull request adds the append functions to the path2 geometry, completing all functionality.

Note: As these functions only apply to the path2 geometry, these should therefore become part of the geometry.

appendArc

[X] code complete
[X] test suite complete, aa% coverage
[X] review complete

appendBezier

[X] code complete
[X] test suite complete, aa% coverage
[X] review complete

appendPoints

[X] code complete
[X] test suite complete, aa% coverage
[X] review complete

z3dev commented 5 years ago

The options to these functions may need some discussion.

@kaosat-dev please review.

z3dev commented 5 years ago

@kaosat-dev

These functions may not be the best, but I don’t think there’s another choice for for V2.

Thoughts?

kaosat-dev commented 5 years ago

@z3dev I am still scratching my head around this one, not a fan indeed, not because of your code , but because this is imperative code, instead of descriptive like the rest of our code : it might be worth taking a look at how is handled in maker.js , I can take a go at possible solutions for this this week, and if that fails, then let's include this, we can always revisit later ?

z3dev commented 4 years ago

@kaosat-dev any luck at a more expressive API?

If not then let’s take this as a priority in the next release.

kaosat-dev commented 4 years ago

@z3dev I have looked a bit into it , but not enough so here is the breakdown I still think it is a very weird / off part of our V1 api : IDEALLY we should follow the same logic as everywhere else:

the user builds its data in an agnostic manner:
- in this case
  - line segments(array [start, end] )
  - quadratic bezier 3 control points, (array [pt1, pt2 pt3])
- cubic bezier curve : 4 control points (array [pt1, pt2, pt3, pt4])
- so arrays of arrays to build a whole path
and THEN you pass that data to our API that creates something specific & optimized
just like we do not have appendPolygon3 etc , it would be weird to have for 2d and a very imperative API instead of declarative

HOWEVER I do not have more time to dig into it, and do not want to postpone/ block things because of that, so do you want us to merge this PR for now ?

z3dev commented 4 years ago

Understood. I think the new API will use these for implementation.

For now, let's take this into V2. If fine, the please merge, @kaosat-dev

z3dev commented 4 years ago

@kaosat-dev please merge. lets go with this initially.