search

joostn / OpenJsCad

3D solid CAD using only Javascript
313 stars 128 forks source link

Signiture of setColor() is inconsist #65

Closed z3dev closed 9 years ago

z3dev commented 9 years ago

The newest version of CSG.js has introduced inconsistency in CSG.prototype.setColor(r,g,b,a) and CSG.Polygon.prototype.setColor([r,g,b,a])

As for the correct signature, I suggest supporting setColor([r,g,b,a]) as the provides the best performance, i.e. the array can be stored directly.

We (CSG.js users) can easily convert from r,g,b,a to [r,g,b,a] as well.

bebbi commented 9 years ago

Tx, I'll look into this. not sure I get the performance argument. Do you plan to submit additional bugs so I can look at it when we have them together?

Spiritdude commented 9 years ago

I recommend to permit both: setColor(r,g,b(,a)) and setColor([r,g,b(,a)])

z3dev commented 9 years ago

Please address this issue first.

The CSG.Shared object retains an array. So, passing the color specification by array will improve performance. Although, permitting both styles would probably be the best for the users of CSG.js.

bebbi commented 9 years ago

@z3dev that's an easy one, can you write a pull request against gh-pages? I'd tend to support the version that's already documented but I'm fine with supporting both for both setColor functions. If I understood you well, I don't believe there's a performance difference in creating an array inside a function call vs inside a function body.

z3dev commented 9 years ago

Hello. I don't have a patch for this.

Please correct and let me know when the new version is available.

joostn commented 9 years ago

See https://github.com/joostn/OpenJsCad/pull/66 I'm proposing to use the array syntax from now on, with the previous syntax supported for backwards compatibility, I don't think performance is relevant here.