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

modify PVector to include better methods for chaining operations #257

Closed
processing-bugs opened this Issue Feb 10, 2013 · 20 comments

Comments

Projects
None yet
7 participants
@processing-bugs

processing-bugs commented Feb 10, 2013

Original author: b...@processing.org (June 07, 2010 01:18:16)

This bug automatically added from:
http://dev.processing.org/bugs/show_bug.cgi?id=1415

Comment from davbol, 2009-12-23 08:49

As of 1.0.9
Now that PVector has become sort of "mainstream", in that for many uses it's no
longer necessary for an end-user to go to an external library for vector/matrix
support, it'd be nice to see it fleshed out a bit more for end-user use.

For example, an obvious ommision is a PVector(PVector source) constructor. Which
currently requires "PVector a = new PVector(); a.set(b);" Similarly, the presence of
set(float []), that implies it'd also be useful to have a PVector(float[] source)
constructor.

The other nicety involves the issue of static methods that return new instances,
versus methods that operate on the original. This setup works fine for discrete
single operations, but becomes cumbersome when stacking operations:

PVector a = new PVector();
PVector b = new PVector(1,2,3);
// c = a + b
PVector c = PVector.add(a,b); // not bad when used like this, but...
println("c = " + c);
// d = (a + (b * 4)) * c
PVector d = PVector.mult(PVector.add(a,PVector.mult(b,4)),c); // getting ugly
println("d = " + d);

Granted, there are alternative ways of wording that code - it's point is just to illustrate
the "wordiness" of creating new instances through stacked operations.

I think I do understand the intent of building it that way, for internal use by core,
reducing memory thrashing of temporary instances, but the end-user might prefer an
alternative syntax.

If the PVector class had a method perhaps named "addNew(PVector v)" (et al) that
created/operated on a new instance, you could instead write:

PVector d = (a.addNew(b).multNew(4)).multNew(c);

That approach is similar to Toxi's vector library (though opposite, where add()
implies a new instance, and addSelf() implies operating internally).

The Java Commons Math Vector3D also defines add() to return a new intance, but
then ALL of their methods return new - there are no "operate internally" methods -
don't really care for that approach either.

And it's not for me to argue about the naming conventions of opSelf vs opNew,
either is fine, but since the existing methods already operate internally, it'd
be "easier" to go with (something like) addNew() and leave add() doing what it
already does.

It's also not easy to just extend PVector as an end-user to implement such ideas
outside of core. It requires a bunch of ugly (and costly) "mangling" code just to gain
this minimal syntactic sugar, f.e.:

class PVectorX extends PVector {
// need to cover some constructors...
PVectorX() { super(); }
PVectorX(float x, float y, float z) { super(x,y,z); }
// this constructor is just to handle "type upgrade" in functions below
// (have to re-wrap the PVector results of static PVector methods into PVectorX's)
PVectorX(PVector v) { this.x=v.x; this.y=v.x; this.z=v.z; }
// then new stuff:
PVectorX addNew(PVector v) { return new PVectorX(PVector.add(this,v)); }
PVectorX multNew(float s) { return new PVectorX(PVector.mult(this,s)); }
PVectorX multNew(PVector v) { return new PVectorX(PVector.mult(this,v)); }
// etc...
}

PVectorX ax = new PVectorX();
PVectorX bx = new PVectorX(1,2,3);
PVectorX cx = ax.addNew(b);
println("cx = " + cx);
PVectorX dx = (ax.addNew(bx).multNew(4)).multNew(cx);
println("dx = " + dx);

(that compiles and runs, btw, but it ain't pretty!)

So it'd be nice to see all variations of input parameters available against all
variations of new/self/staticnew calling conventions on all methods (as applicable).

And, has been mentioned on the forum by others, fe:
http://processing.org/discourse/yabb2/YaBB.pl?num=1259250607
PVector could be made available as argument to many other methods. But that's
almost deserving of a separate topic and its own enh req.

Anyway, just some thoughts/ideas to put on a back burner somewhere.
Cheers

Original issue: http://code.google.com/p/processing/issues/detail?id=218

@processing-bugs

This comment has been minimized.

Show comment
Hide comment
@processing-bugs

processing-bugs Feb 10, 2013

From b...@processing.org on June 07, 2010 01:18:16
Comment from fry, 2009-12-24 06:38

Thanks for the note. Do you have an idea of what the full list of methods
would look like? You've clearly given this some thought and I like the
direction.

For a copy of a vector, use get(), just like the image methods. I think
copy() also works.

You're correct re: the static methods, but I'm not really comfortable with
the general level of consistency in how they're implemented. In the end, it
may be too clever to overload add() in so many ways, and that a different
addStatic() method should be used instead for the efficient versions just
to keep them all straight.

For the other methods, I think we have another bug report for that (or it
was appended to another like here), but the issue is that it affects an
enormous number of methods (you'd be surprised to see the list), so I've
been avoiding expanding the function count that much.

processing-bugs commented Feb 10, 2013

From b...@processing.org on June 07, 2010 01:18:16
Comment from fry, 2009-12-24 06:38

Thanks for the note. Do you have an idea of what the full list of methods
would look like? You've clearly given this some thought and I like the
direction.

For a copy of a vector, use get(), just like the image methods. I think
copy() also works.

You're correct re: the static methods, but I'm not really comfortable with
the general level of consistency in how they're implemented. In the end, it
may be too clever to overload add() in so many ways, and that a different
addStatic() method should be used instead for the efficient versions just
to keep them all straight.

For the other methods, I think we have another bug report for that (or it
was appended to another like here), but the issue is that it affects an
enormous number of methods (you'd be surprised to see the list), so I've
been avoiding expanding the function count that much.

@processing-bugs

This comment has been minimized.

Show comment
Hide comment
@processing-bugs

processing-bugs Feb 10, 2013

From b...@processing.org on June 07, 2010 01:18:17
Comment from davbol, 2009-12-24 08:37

I hesitate to offer too much design suggestions, other than to just bring up issue for
better minds to ponder, because I'd been using Java Commons Math for so long that
I'm still trying to unlearn its way of doing things.

For example, with JCM even normalize() returns new. So you (often) find yourself
writing self-referential assignements like: a=a.normalize();
Because writing just a.normalize(); does nothing, result evaporates into the ether!

I think that approach might be even more confusing to p5's core audience. which is
why i largely agree that the default ops (that is, with the simple name, add() fe)
should be the operate internally versions (as is already the case).

So, to be clear, it's only the naming/calling conventions of the "return new PVector"
methods that I'd suggest need some consideration. Currently, those are: add(), sub
(), mult(), div(), cross().

Though, in addition, if you do adopt a opNew() approach, a couple of "in place only"
methods should be "upgraded" to offer an opNew() version in case you WANT to work
like JCM and leave the original intact: normalize(), limit().

A related side issue is that everything that implies a PVector result should return
PVector. Even the "operate internally" versions (they'd return "this"). So all
operations can be chained in a single statement, fe: a.normalize().cross(x); (i'd
call this a side issue, because this could be done in isolation even if nothing else was)

And those couple extra constructors, just for convenience for those of us who tend to
forget about get() :-D.

I CAN offer critique on static methods that return float though...

For example, why is there a static float dist(PVector v1,PVector v2)? In order to call
that successfully you MUST have instances, so either v1.dist(v2) or v2.dist(v1) would
have worked instead, at least in every case that I can imagine. The static dist()
does NOT check for nulls and create a zero-vector in its place - at first that's what i
thought the intent might have been, but nope. Same with static dot() and
angleBetween().

(btw, it doesn't really matter, but i notice a couple parenthetical math typos in my
initial post, just in case anyone ever tries to validate between the two code blocks)

cheers, and merry xmas!!

processing-bugs commented Feb 10, 2013

From b...@processing.org on June 07, 2010 01:18:17
Comment from davbol, 2009-12-24 08:37

I hesitate to offer too much design suggestions, other than to just bring up issue for
better minds to ponder, because I'd been using Java Commons Math for so long that
I'm still trying to unlearn its way of doing things.

For example, with JCM even normalize() returns new. So you (often) find yourself
writing self-referential assignements like: a=a.normalize();
Because writing just a.normalize(); does nothing, result evaporates into the ether!

I think that approach might be even more confusing to p5's core audience. which is
why i largely agree that the default ops (that is, with the simple name, add() fe)
should be the operate internally versions (as is already the case).

So, to be clear, it's only the naming/calling conventions of the "return new PVector"
methods that I'd suggest need some consideration. Currently, those are: add(), sub
(), mult(), div(), cross().

Though, in addition, if you do adopt a opNew() approach, a couple of "in place only"
methods should be "upgraded" to offer an opNew() version in case you WANT to work
like JCM and leave the original intact: normalize(), limit().

A related side issue is that everything that implies a PVector result should return
PVector. Even the "operate internally" versions (they'd return "this"). So all
operations can be chained in a single statement, fe: a.normalize().cross(x); (i'd
call this a side issue, because this could be done in isolation even if nothing else was)

And those couple extra constructors, just for convenience for those of us who tend to
forget about get() :-D.

I CAN offer critique on static methods that return float though...

For example, why is there a static float dist(PVector v1,PVector v2)? In order to call
that successfully you MUST have instances, so either v1.dist(v2) or v2.dist(v1) would
have worked instead, at least in every case that I can imagine. The static dist()
does NOT check for nulls and create a zero-vector in its place - at first that's what i
thought the intent might have been, but nope. Same with static dot() and
angleBetween().

(btw, it doesn't really matter, but i notice a couple parenthetical math typos in my
initial post, just in case anyone ever tries to validate between the two code blocks)

cheers, and merry xmas!!

@processing-bugs

This comment has been minimized.

Show comment
Hide comment
@processing-bugs

processing-bugs Feb 10, 2013

From b...@processing.org on June 07, 2010 01:18:17
Comment from fry, 2009-12-24 08:56

Sure, this is very helpful. I'm gonna disappear for a few days but will
give it more thought in the new year. It'd be nice to get PVector in better
shape, and I think it's not much work, it's just a matter of being a little
smarter about it.

Merry Christmas to you as well! Take care.

processing-bugs commented Feb 10, 2013

From b...@processing.org on June 07, 2010 01:18:17
Comment from fry, 2009-12-24 08:56

Sure, this is very helpful. I'm gonna disappear for a few days but will
give it more thought in the new year. It'd be nice to get PVector in better
shape, and I think it's not much work, it's just a matter of being a little
smarter about it.

Merry Christmas to you as well! Take care.

@processing-bugs

This comment has been minimized.

Show comment
Hide comment
@processing-bugs

processing-bugs Feb 10, 2013

From ironside...@gmail.com on November 18, 2010 10:14:42
Can I add a +1 for, at the very least, the so-called "internal operation" methods to return "this". Chaining PVector methods would be a real time/space-saver.

Maybe I'm being woefully naive, but I can't think of any backward-compatibility issues with adding that functionality now; maybe someone can un/confirm this? If so, and I'm not sure on the process here, I'm more than happy to make the fix

processing-bugs commented Feb 10, 2013

From ironside...@gmail.com on November 18, 2010 10:14:42
Can I add a +1 for, at the very least, the so-called "internal operation" methods to return "this". Chaining PVector methods would be a real time/space-saver.

Maybe I'm being woefully naive, but I can't think of any backward-compatibility issues with adding that functionality now; maybe someone can un/confirm this? If so, and I'm not sure on the process here, I'm more than happy to make the fix

@processing-bugs

This comment has been minimized.

Show comment
Hide comment
@processing-bugs

processing-bugs Feb 10, 2013

From f...@processing.org on November 22, 2010 00:44:49
Sadly it does break backward compatibility with any libraries/compiled code that uses PVector. This is the holdup, really, that we'd need to decide a good time to make the change, and be 100% sure about any modifications.

Also, the problem with chaining operations is a lot of temporary objects being created. This can greatly slow down sketches, moreso on Android than on desktops, but it can be an issue. This is the reason for the current architecture that tries to avoid creating too many temp objects.

processing-bugs commented Feb 10, 2013

From f...@processing.org on November 22, 2010 00:44:49
Sadly it does break backward compatibility with any libraries/compiled code that uses PVector. This is the holdup, really, that we'd need to decide a good time to make the change, and be 100% sure about any modifications.

Also, the problem with chaining operations is a lot of temporary objects being created. This can greatly slow down sketches, moreso on Android than on desktops, but it can be an issue. This is the reason for the current architecture that tries to avoid creating too many temp objects.

@processing-bugs

This comment has been minimized.

Show comment
Hide comment
@processing-bugs

processing-bugs Feb 10, 2013

From elj...@gmail.com on July 26, 2011 15:33:20
Hello. I'm interested in contributing to this project. Especially to PVector. No offense intended, but this class needs some love. I already have my first patch ready in my "read-only-processing" svn client, but I'm not sure how you folks like outsiders to contribute code. I couldn't find it documented.

processing-bugs commented Feb 10, 2013

From elj...@gmail.com on July 26, 2011 15:33:20
Hello. I'm interested in contributing to this project. Especially to PVector. No offense intended, but this class needs some love. I already have my first patch ready in my "read-only-processing" svn client, but I'm not sure how you folks like outsiders to contribute code. I couldn't find it documented.

@ElectricJack

This comment has been minimized.

Show comment
Hide comment
@ElectricJack

ElectricJack Apr 19, 2013

In my take on a vector class, there is the option to call operations that create temporary variables to keep code more readable, or operate on them incrementally, if speed is of greater importance. Almost all of the functions are chain-able.

https://github.com/ElectricJack/FieldFXLibs/blob/master/src/com/fieldfx/math/Vector3.java

Perhaps consider something along these lines for future implementations.

ElectricJack commented Apr 19, 2013

In my take on a vector class, there is the option to call operations that create temporary variables to keep code more readable, or operate on them incrementally, if speed is of greater importance. Almost all of the functions are chain-able.

https://github.com/ElectricJack/FieldFXLibs/blob/master/src/com/fieldfx/math/Vector3.java

Perhaps consider something along these lines for future implementations.

@shiffman

This comment has been minimized.

Show comment
Hide comment
@shiffman

shiffman May 14, 2013

Member

I think it would be nice to support chaining for advanced users as long as we don't lose the simplicity and performance advantages of what we have. And don't break everything that's already using PVector. I'll look into this more soon (probably post 2.0)

Member

shiffman commented May 14, 2013

I think it would be nice to support chaining for advanced users as long as we don't lose the simplicity and performance advantages of what we have. And don't break everything that's already using PVector. I'll look into this more soon (probably post 2.0)

@jordanorelli

This comment has been minimized.

Show comment
Hide comment
@jordanorelli

jordanorelli Feb 3, 2014

Contributor

is there any reason why we couldn't simply return this in all of the simple math functions that currently have a void return type? I don't understand how that could break anything (of course, I may simply be wrong). What's wrong with just doing this? It seems to work when I run it but it also sounds like this has been suggested and there's thought to be a reason why it's a bad idea, I'm just having a hard time understanding how this would break anything.

Contributor

jordanorelli commented Feb 3, 2014

is there any reason why we couldn't simply return this in all of the simple math functions that currently have a void return type? I don't understand how that could break anything (of course, I may simply be wrong). What's wrong with just doing this? It seems to work when I run it but it also sounds like this has been suggested and there's thought to be a reason why it's a bad idea, I'm just having a hard time understanding how this would break anything.

@ElectricJack

This comment has been minimized.

Show comment
Hide comment
@ElectricJack

ElectricJack Feb 7, 2014

Yes returning this instead of void won't break anything. Might not even need to be in the lightweight docs if the goal is to keep things simple for beginners.

ElectricJack commented Feb 7, 2014

Yes returning this instead of void won't break anything. Might not even need to be in the lightweight docs if the goal is to keep things simple for beginners.

@shiffman

This comment has been minimized.

Show comment
Hide comment
@shiffman

shiffman Feb 11, 2014

Member

Thanks for re-opening this thread. One major concern is backwards compatibility for all Processing libraries. I'm currently investigating.

Member

shiffman commented Feb 11, 2014

Thanks for re-opening this thread. One major concern is backwards compatibility for all Processing libraries. I'm currently investigating.

@clankill3r

This comment has been minimized.

Show comment
Hide comment
@clankill3r

clankill3r May 4, 2014

Returning this would be nice.
How's the investigation going? :)

clankill3r commented May 4, 2014

Returning this would be nice.
How's the investigation going? :)

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Jun 5, 2014

Member

Provisionally adding for 3.0.

Member

benfry commented Jun 5, 2014

Provisionally adding for 3.0.

@GoToLoop

This comment has been minimized.

Show comment
Hide comment
@GoToLoop

GoToLoop Jun 15, 2014

Congratz on the reference chaining decision! 👏
Wanna see it in more places! 👯

GoToLoop commented Jun 15, 2014

Congratz on the reference chaining decision! 👏
Wanna see it in more places! 👯

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Jul 25, 2014

Member

Had to remove this because it changes the method signature, breaking any pre-compiled code that uses it.

We're going to consider re-adding it later in the 3.0 process if there are enough other things we're breaking, but for now, it broke controlp5 right off the bat, and we've been getting bitten by it in day-to-day use.

Member

benfry commented Jul 25, 2014

Had to remove this because it changes the method signature, breaking any pre-compiled code that uses it.

We're going to consider re-adding it later in the 3.0 process if there are enough other things we're breaking, but for now, it broke controlp5 right off the bat, and we've been getting bitten by it in day-to-day use.

@clankill3r

This comment has been minimized.

Show comment
Hide comment
@clankill3r

clankill3r Jul 25, 2014

In my opinion it's better to break any pre-compiled code at the launch of 3.0 then down the road at some 3.x

clankill3r commented Jul 25, 2014

In my opinion it's better to break any pre-compiled code at the launch of 3.0 then down the road at some 3.x

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Jul 25, 2014

Member

That's the plan.

Member

benfry commented Jul 25, 2014

That's the plan.

@clankill3r

This comment has been minimized.

Show comment
Hide comment
@clankill3r

clankill3r Jul 25, 2014

ah ok :) I'm a bit drunk :)

clankill3r commented Jul 25, 2014

ah ok :) I'm a bit drunk :)

@clankill3r

This comment has been minimized.

Show comment
Hide comment
@clankill3r

clankill3r Jun 10, 2015

While the gates of hell are still open I have to say something about this:

It's also not easy to just extend PVector as an end-user to implement such ideas
outside of core. It requires a bunch of ugly (and costly) "mangling" code just to gain
this minimal syntactic sugar, f.e.:

class PVectorX extends PVector {
// need to cover some constructors...
PVectorX() { super(); }
PVectorX(float x, float y, float z) { super(x,y,z); }
etc. etc. etc.

This is not the case if the class will be structured like this:

class PVector<C extends PVector> {

   C add(float x, float y, float z) {
    this.x += x;
    this.y += y;
    this.z += z;
    return (C) this;    
  } 
}

Here is an example that works in the pde.

void setup() {

  PVec3 a = new PVec3();
  a.add(1,2,3).add(4,5,6);

  println(a.x, a.y, a.z);

  PVec3X b = new PVec3X();
  b.add(1,2,3).someOtherMethod().add(4,5,6);
  println(b.x, b.y, b.z);  
}

class PVec3<C extends PVec3> {

  float x, y, z;

  C add(float x, float y, float z) {
    this.x += x;
    this.y += y;
    this.z += z;
    return (C) this;    
  } 
}


class PVec3X extends PVec3<PVec3X> {

  PVec3X someOtherMethod() {
    println("we use add after calling someOtherMethod!");
    return this;    
  }  
}

I think doing it like that would be really great cause it would allow for a particle library for example that can still work with PVector.

clankill3r commented Jun 10, 2015

While the gates of hell are still open I have to say something about this:

It's also not easy to just extend PVector as an end-user to implement such ideas
outside of core. It requires a bunch of ugly (and costly) "mangling" code just to gain
this minimal syntactic sugar, f.e.:

class PVectorX extends PVector {
// need to cover some constructors...
PVectorX() { super(); }
PVectorX(float x, float y, float z) { super(x,y,z); }
etc. etc. etc.

This is not the case if the class will be structured like this:

class PVector<C extends PVector> {

   C add(float x, float y, float z) {
    this.x += x;
    this.y += y;
    this.z += z;
    return (C) this;    
  } 
}

Here is an example that works in the pde.

void setup() {

  PVec3 a = new PVec3();
  a.add(1,2,3).add(4,5,6);

  println(a.x, a.y, a.z);

  PVec3X b = new PVec3X();
  b.add(1,2,3).someOtherMethod().add(4,5,6);
  println(b.x, b.y, b.z);  
}

class PVec3<C extends PVec3> {

  float x, y, z;

  C add(float x, float y, float z) {
    this.x += x;
    this.y += y;
    this.z += z;
    return (C) this;    
  } 
}


class PVec3X extends PVec3<PVec3X> {

  PVec3X someOtherMethod() {
    println("we use add after calling someOtherMethod!");
    return this;    
  }  
}

I think doing it like that would be really great cause it would allow for a particle library for example that can still work with PVector.

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Jun 10, 2015

Member

That's far, far too complicated for the target audience of PVector. And we never use templating anywhere in the API, because it's much too obtuse for beginners (relative to its actual benefits).

The class is there for something simple. Immediately past that, people can make their own versions of it (the code is of course available), or subclass it in more basic ways.

This is also not the place for discussions about PVector. This issue is about chaining operations, which is now implemented. Other feature requests should be filed elsewhere.

Member

benfry commented Jun 10, 2015

That's far, far too complicated for the target audience of PVector. And we never use templating anywhere in the API, because it's much too obtuse for beginners (relative to its actual benefits).

The class is there for something simple. Immediately past that, people can make their own versions of it (the code is of course available), or subclass it in more basic ways.

This is also not the place for discussions about PVector. This issue is about chaining operations, which is now implemented. Other feature requests should be filed elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment