Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSG.Path2D.append{Point,Points,Bezier} are inconsistent and underdocumented #165

Closed
fischman opened this issue Aug 9, 2016 · 6 comments
Closed

Comments

@fischman
Copy link
Contributor

fischman commented Aug 9, 2016

CSG.Path2D's appendPoints and appendBezier methods are not mentioned in https://en.wikibooks.org/wiki/OpenJSCAD_User_Guide (only appendPoint (singular) is documented). More confusingly, appendBezier and appendPoint don't modify their |this| parameter, while appendPoints (plural) does. It would be nice if:

  • all methods were documented; and
  • appendFoo methods were all consistent one way or the other about whether they affect |this| or whether their return value needs to be assigned back to the callee. (IMO appendFoo should affect their callee)

Code below demonstrates the issue in pairs: bezier in 1&2, point in 3&4, points in 5&6.

function test1() {
    var path = new CSG.Path2D([[0,0], [0, 5]]);
    path = path.appendBezier([null, [10,0]]);
    return path.close().innerToCAG();
}

function test2() {
    var path = new CSG.Path2D([[0,0], [0, 5]]);
    /* path = */ path.appendBezier([null, [10,0]]);
    return path.close().innerToCAG();
}

function test3() {
    var path = new CSG.Path2D([[0,0], [0, 5]]);
    path = path.appendPoint([10,0]);
    return path.close().innerToCAG();
}

function test4() {
    var path = new CSG.Path2D([[0,0], [0, 5]]);
    /* path = */ path.appendPoint([10,0]);
    return path.close().innerToCAG();
}

function test5() {
    var path = new CSG.Path2D([[0,0], [0, 5]]);
    path = path.appendPoints([[10,0]]);
    return path.close().innerToCAG();
}

function test6() {
    var path = new CSG.Path2D([[0,0], [0, 5]]);
    /* path = */ path.appendPoints([[10,0]]);
    return path.close().innerToCAG();
}

function main() {
    // return test1();  // PASS
    // return test2();  // FAIL: Uncaught Error: CAG shape needs at least 3 points
    // return test3();  // PASS
    // return test4();  // FAIL: Uncaught Error: CAG shape needs at least 3 points
    // return test5();  // PASS
    return test6();  // PASS!
}

Thanks for providing this great resource, BTW :)

@fischman
Copy link
Contributor Author

fischman commented Aug 9, 2016

Also, appendArc() is undocumented.

@z3dev
Copy link
Member

z3dev commented Aug 11, 2016

@fischman thanks, but we would like our fans to change the documentation as well. it's now available on Wikibooks, so go for it!

https://en.wikibooks.org/wiki/OpenJSCAD_User_Guide

@fischman
Copy link
Contributor Author

What about the behavior? Do you simply want the inconsistency documented
or do you want to resolve it one way or the other?

On Wed, Aug 10, 2016, 6:25 PM Z3 Development notifications@github.com
wrote:

@fischman https://github.com/fischman thanks, but we would like our
fans to change the documentation as well. it's now available on Wikibooks,
so go for it!

https://en.wikibooks.org/wiki/OpenJSCAD_User_Guide


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#165 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAG-JI76NWmALb27zr4D6_2rYgRw893oks5qenn5gaJpZM4Jgjos
.

@z3dev
Copy link
Member

z3dev commented Aug 12, 2016

We'll have to discuss the inconsistencies with Joost as the CSG library is imported from another project. However, I believe that CSG and CAG functions should always return a new object, rather than modifying the original. The documentation from Joost also suggests this as well.

http://joostn.github.io/OpenJsCad/

@fischman
Copy link
Contributor Author

@z3dev
Copy link
Member

z3dev commented Feb 17, 2017

This issue was moved to jscad/csg.js#16

@z3dev z3dev closed this as completed Feb 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants