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

Redesigning Vec family API #48

Closed
soypat opened this issue Jan 16, 2022 · 8 comments
Closed

Redesigning Vec family API #48

soypat opened this issue Jan 16, 2022 · 8 comments

Comments

@soypat
Copy link
Contributor

soypat commented Jan 16, 2022

Using the method approach to managing vectors feels clunky and less readable than defining functions that take all vectors needed as arguments.

Compare

center := a.f(t) 
n := a.f(t + dt2)
n = n.Sub(a.f(t - dt2))
pv := p.Sub(center) 

with

center := a.f(t)
n := sdfv.Sub(a.f(t + dt2),  a.f(t - dt2))
pv := sdfv.Sub(p, center) 

If this were to be done it could be moved to a subpackage and the methods be left for backwards compatibility

@deadsy
Copy link
Owner

deadsy commented Jan 18, 2022

So:
func Sub(a, b V3) V3

Instead of:
func (a V3) Sub(b V3) V3

The advantage of the second form is that it leaves "Sub" free in the namespace to be used for other types. E.g. V2, integer vectors, etc.

I think that's why I selected it. Blame a lack of function templates in go.

@soypat
Copy link
Contributor Author

soypat commented Jan 18, 2022

What you are saying is reasonable. This namespace constraint however can be solved by creating a package for each type much like gonum does.

@deadsy
Copy link
Owner

deadsy commented May 4, 2022

This namespace constraint however can be solved by creating a package for each type

Done.

10ae73b

I haven't created an c = op(a, b) functions at this time, but it is trivial to do so.

@soypat
Copy link
Contributor Author

soypat commented May 4, 2022

Awesome! I guess as soon as those functions are written we can close this issue. I am unavailable to contribute this feature.

@aprice2704
Copy link

aprice2704 commented Aug 1, 2022

@soypat you could however do:

n := a.f(t + dt2).Sub(a.f(t - dt2))`

couldn't you?

@soypat
Copy link
Contributor Author

soypat commented Aug 3, 2022

@aprice2704 my issue is not with the amount of lines it takes to express an operation but rather how readable the code is. I dont think methods as operations lend themeselves to readable code, in my experience

@aprice2704
Copy link

I understand but respectfully disagree :) I find the method-operator version to be clearer, since it enforces the order in which operators are applied rather than relying on conventions from natural languages.
sub(a,b) results in a-b or b-a?
a.sub(b) must be a-b.
I have been writing a fairly complex set of geometry manipulations and found the operator-method to be quite clear. :)
In any event, the methods being present do not constrain you from adding sub(a,b) and friends to your own packages if you wish; however, adding methods requires forking the package, so I would much prefer @deadsy leaves them in place :)

@deadsy
Copy link
Owner

deadsy commented Sep 27, 2022

At this point the vector packages have been split out into individual packages for each vector type. The rest of the code has been converted to use the new packages.

At the moment the a.Op(b) for is still the main way of doing things.

The Op(a,b) form is possible without leading to name space problems.
E.g. v2.Add(a,b)

Still- I haven't created these functions, because I don't feel the need.

@deadsy deadsy closed this as completed Sep 27, 2022
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