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

make final decision about arities of various protocol methods #38

Closed
postspectacular opened this issue Apr 9, 2016 · 4 comments
Closed

Comments

@postspectacular
Copy link
Member

Currently common protocol methods like translate, scale and the various basic vector math ops allow specifying coordinate args as vectors or individual coords, e.g.

(g/translate (a/aabb 1) (vec3 10 20 30))
(g/translate (a/aabb 1) [10 20 30])
(g/translate (a/aabb 1) 10 20 30)

About the last case:

Pros:

  • in some cases avoids construction of temp vectors, though most internal usage doesn't make use of this arity option, since args are mostly vectors already

Cons:

  • considerable additional effort required for maintaining 3-arities of each implementation
  • currently missing implementations of this aritiy in various types
  • extra argument type checking required for last case

Proposal:

  • remove protocol 4-arg arity (w/ individual coords) from protocols and enforce usage of vector args

Thoughts?

@jackrusher
Copy link

I advocate simplicity/consistency/ease-of-maintenance over the slight potential performance increase gained by maintaining the extra arities.

@vl4dimir
Copy link

+1, what Jack said.

On Sunday, 10 April 2016, Jack Rusher notifications@github.com wrote:

I advocate simplicity/consistency/ease-of-maintenance over the slight
potential performance increase gained by maintaining the extra arities.


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#38 (comment)

@postspectacular
Copy link
Member Author

I love consensus! :) Will schedule this for week after next...

@postspectacular
Copy link
Member Author

Okay guys, it's done:
a516bd5 + the 2 following commits to update tests/demos

will build another snapshot this week

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

3 participants