-
Notifications
You must be signed in to change notification settings - Fork 9
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
API style direction discussion #2
Comments
result.Operation(args) is pretty common from the math libs I've used. It sort of depends on if you want immutability or not, and with Go's concurrency I suppose that is worth considering. But yeah, you would suffer from more copying if you went for immutability. I'm not sure what you mean when you talk about adding a Get prefix. If you have a *Vector3 and you call its Length method, surely it would be clear enough that you are calculating the length of that vector? I don't think you should feel responsible for making things easier for people not familiar with the concept of object methods. I don't know, maybe I'm missing something there. I think you are leaning towards the right design. It should be familiar and easy to pick up for most people. |
I agree. You shouldn't have to babysit your users, every api takes some
|
I'm curious as to where you are with this. I'm starting to use the library more and more, and want to be careful about needing to refactor too much code if I want to use the latest. |
After taking numerous stabs at this, I've gone through and switched to using return values again rather than either passing an argument or making them member methods. The big deciders for me were (a) my original statement of intent I made way back in a Google+ post said I'd do it that way, and (b) it was making the godoc documentation really painful to follow. My reasoning is that if you want performance, you'll probably be using gcc or (eventually) llgo instead. Are you finding the current interface useful? If so, I may kick that over into a v1 subdir so it doesn't go away. Note that I have found one copy/paste error for one of the functions in my refactoring travels. I keep meaning to check that in separately.. :( |
I wouldn't make a v1 subdir. If I want a specific version of the API, you shouldn't need to keep it around in a separate folder. I can just fork an old revision and use that. I may end up doing that. Do you have some benchmark results on the differences in performance? |
I did some benchmarking with V3Add at one point.. I seem to recall that the overhead from passing arguments in and out of the function dwarfs the computation time-wise, so anything changed in the function signature is going to be noticeable with the gc compiler. I didn't keep any numbers from those tests, though. Looking at my benchmark files, it looks like the last thing I tested different implementations of was M4Add: func M4Add(a, b *Matrix4) Matrix4 = 138ns/op In this case at least, there's enough math in the function that it doesn't get eclipsed by the cost of passing the arguments. The first signature in that list is what I'm switching to for the moment, though I haven't ruled out eliminating the use of pointers entirely for the simpler types (Vectors, Points), should gcc and/or llgo be found to optimize those variables into an SSE register. (Tests conducted on a Late 2008 Aluminum Macbook w/ 4 GB RAM using gc Go 1.0.3) |
Mind committing the copy / paste error you were mentioning above, or sending a patch file against the current repository? |
Just pushed. I think that was the only one I found (GetCol), but I'll let you know if I notice any others. |
Ahoy-hoy. I'm planning on making some broad-strokes changes to the API to make it a little better organized. Currently the majority of functions are just plain old functions, and I'd like to make them type methods instead.
The style I'm strongly leaning towards at the moment is the one used in go/misc/cgo/gmp/gmp.go:
resultVar.Operation(argument, argument, ...)
This is instead of the more conventional:
resultVar = argument.Operation(argument)
The idea is that by having the destination be the argument you're operating on, you can avoid having to pass the result back on the stack (performance win, since it puts the onus of handling the case where the src and dst are the same variable on the library, instead of depending on an implied copy). I also like the way the arguments and operation are ordered, but that may just be personal preference and my familiarity with assembly language syntax kicking in.
The main downsides I can see for this are:
(a) confusion, from a programmer looking for the appropriate function perspective, when looking for simple functions that take a single argument like Length or Determinant. In a sea of Add, Sub, Mul, etc. where the convention is to have it operate on the destination, these may be confusing to beginners. The obvious solution here would be to preface the ones that operate more conventionally with Get. e.g., vec.GetLength() and mat.GetDeterminant().
One alternative might be to declare a Float type and then have methods that operate on it, e.g., func (f Float) DotV3(a, b *Vector3). But I'm not particularly enthusiastic about forcing people to use a different float type, or add boilerplate conversion code just to get basic data like that out of the library. (As consistant as it might make the library overall.) (The type declaration would be required in order for this to work, as you can't declare type methods for internal types like float32)
(b) inconsistency arising from (a) with respect to things like dot products. In order to avoid making my own Float, I'll probably end up having to make a function with signature: func (v *Vector3) Dot(a *Vector3) float32.
(c) inconsistency from the Go language as a whole. I understand that the language as a whole follows what I've above dubbed the conventional style, so making it different is bound to cause people headaches. For the moment, this approach may be faster, but two-three years down the road, will that still be the case? (Is it even the case today with the gcc front end?)
Comments? Thoughts?
The text was updated successfully, but these errors were encountered: