Bug in Vector.lerp() implementation

xuanquang1999 commented 1 year ago

There seems to be a bug with the implementation of Vector.lerp() function

$.Vector.lerp= function(v,u,t){return new $.Vector(v.x * (1-t) + u.x * t,
                                                   v.y = v.y * (1-t) + u.y * t,
                                                   v.z = v.z * (1-t) + u.z * t);}

The function change the value of y and z coordinate of the first param vector, which don't seems to be intended (it should not modify any value of the first param vector)

quinton-ashley commented 1 year ago

Is the solution to just to remove v.y = and v.z = ?

Could you also please show an example of correct output for this function?

GoToLoop commented 1 year ago

p5js' lerp() reference: https://p5js.org/reference/#/p5.Vector/lerp

My Processing Pjs-based implementation:

quinton-ashley commented 1 year ago

My solution to this was just to do this, ha! This is technically faster than what @LingDong- had before as well, creating new objects takes longer than just calling the function on the first vector inputted.

Vector.add = (v, u) => v.add(u);
Vector.rem = (v, u) => v.rem(u);
Vector.sub = (v, u) => v.sub(u);
Vector.mult = (v, u) => v.mult(u);
Vector.div = (v, u) => v.div(u);
Vector.dist = (v, u) => v.dist(u);
Vector.cross = (v, u) => v.cross(u);
Vector.lerp = (v, u, t) => v.lerp(u, t);
Vector.equals = (v, u, epsilon) => v.equals(u, epsilon);

The lerp implementation in the Vector class looks correct to me.

https://github.com/quinton-ashley/q5js

GoToLoop commented 1 year ago

, creating new objects takes longer than just calling the function on the first vector inputted.

Static methods aren't supposed to mutate its passed arguments! 🙅

Take for example this implementation of static method PVector.add(): 👓 https://Gist.GitHub.com/GoToLoop/acbf106aa784820aff23#file-pvector-js-L56-L58

PVector.add = (v1, v2, t) =>
  t? t.set(v1.x + v2.x, v1.y + v2.y, v1.z + v2.z)
   : new PVector(v1.x + v2.x, v1.y + v2.y, v1.z + v2.z);

Neither parameters v1 & v2 are ever mutated there. 🔒

Instead, either a new PVector is created or the method relies on parameter t to store the result.

BtW, file PVector.js is a close implementation of class PVector from Processing Java, which p5js more or less follows. ☕

Actually, p5js' implementation of its static method p5.Vector.add() also has a 3rd parameter that serves the same purpose as Java Mode's PVector.add(): 😻 https://GitHub.com/processing/p5.js/blob/v1.5.0/src/math/p5.Vector.js#L2070-L2084

quinton-ashley commented 1 year ago

ahh! okay I understand. I'll have to revert those changes

On Sun, Feb 12, 2023 at 22:40 GoToLoop @.***> wrote:

, creating new objects takes longer than just calling the function on the first vector inputted.

Static methods aren't supposed to mutate its passed arguments! 🙅

Take for example this implementation of static method PVector.add(): 👓

https://Gist.GitHub.com/GoToLoop/acbf106aa784820aff23#file-pvector-js-L56-L58

PVector.add = (v1, v2, t) =>

t? t.set(v1.x + v2.x, v1.y + v2.y, v1.z + v2.z)

: new PVector(v1.x + v2.x, v1.y + v2.y, v1.z + v2.z);

Neither parameters v1 & v2 are ever mutated there. 🔒

Instead, either a new PVector is created or the method relies on parameter t to store the result.

BtW, file PVector.js https://gist.github.com/GoToLoop/acbf106aa784820aff23#file-pvector-js is a close implementation of class PVector from Processing Java, which p5js more or less follows. ☕

Actually, p5js' implementation of its p5.Vector.add() also has a 3rd parameter that serves the same purpose as Java Mode's PVector.add(). 😻

— Reply to this email directly, view it on GitHub https://github.com/LingDong-/q5xjs/issues/21#issuecomment-1427290787, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEY2RQ3AGN2CWWUZ5GZRVV3WXGUJPANCNFSM6AAAAAAUUZKB5M . You are receiving this because you commented.Message ID: @.***>

quinton-ashley commented 1 year ago

Done, thank you! I still need to fix lerp though.

GoToLoop commented 1 year ago

Have you looked at my PVector.lerp() implementation? Although it's indeed a little complex b/c the whole thing is divided in 3 parts:

The lerp() calculation itself.
The static implementation which invokes the instance implementation.
The instance implementation which may invoke its static counterpart when arguments is 3.

quinton-ashley commented 1 year ago

Not yet and clearly this is an area of the project I know very little about 😅

Do you think you'd be able to make the fix and do a pull request to my repo? https://github.com/quinton-ashley/q5.js

GoToLoop commented 1 year ago

I haven't created any of those vector methods. I've just refactored an existing code.
What I can do is refactor again file "PVector.js" to modern JS and make sure it fits in q5.js.
Just noticed the whole vector implementation in q5.js is memory intensive!
Every time we instantiate a new vector in q5.js all of its methods are re-created!
B/c its methods are defined inside the vector constructor itself instead of its prototype{}!
ES6 classes automatically place methods inside their prototype{} btW.
Anyways, I'm gonna leave you w/ a shorter version w/ just the needed parts for the lerp() implementation, which can be copied & pasted on any browser's console for test purposes:

const
  lerp = (start, stop, amt = 0) => +start + amt*(stop - start),

  argsErr = (mtd, len, min) => {
    throw `Too few args passed to ${mtd}() [${len} < ${min}].`;
  };

function PVector(x, y, z) {
  this.x = x || 0, this.y = y || 0, this.z = z || 0;
}

PVector.lerp = (v1, v2, amt, t) => (t && t.set(v1) || v1.copy()).lerp(v2, amt);

PVector.prototype.lerp = function (a, b, c, d) {
  let x, y, z, amt;
  const len = arguments.length;

  if (len < 2)  argsErr('lerp', len, 2);

  if (len === 2) { // given vector and amt
    ({x, y, z} = a), amt = b;

  } else if (len === 3) { // given vector 1, vector 2 and amt
    return PVector.lerp(a, b, c);

  } else { // given x, y, z and amt
    [x, y, z, amt] = arguments;
  }

  return this.set(lerp(this.x, x, amt),
                  lerp(this.y, y, amt),
                  lerp(this.z, z, amt));
};

PVector.prototype.copy = function () {
  return new PVector(this.x, this.y, this.z);
};

PVector.prototype.set = function (v, y, z) {
  if (y != void 0)  this.x = +v, this.y = +y, z != void 0 && (this.z = +z);
  else this.set(v[0] || v.x || 0, v[1] || v.y || 0, v[2] || v.z);
  return this;
};

quinton-ashley commented 1 year ago

Just noticed the whole vector implementation in q5.js is memory intensive! Every time we instantiate a new vector in q5.js all of its methods are re-created!

That's true, I will change it to use an ES6 class!

GoToLoop commented 1 year ago

That's true, I will change it to use an ES6 class!

Actually I'm doing it right now. 😆 Just starting though: 👲

"use strict";

function Q5(scope, parent) {
  const $ = this;

  $.createVector = (x, y, z) => new $.Vector(x, y, z);

  Q5.Vector = $.Vector = class Vector {
    constructor(x, y, z) {
      this.x = x || 0, this.y = y || 0, this.z = z || 0;
      this._deleteMagCache();
    }

    _deleteMagCache() {
      this._magSq = this._mag = null;
    }

    _calcMagCache() {
      if (this._magSq == null)
        this._magSq = (this._mag = Math.hypot(this.x, this.y, this.z)) ** 2;
    }
  };
}

quinton-ashley commented 1 year ago

Just finished and I tested it this time! I moved the Vector class to the bottom of the file and now it is only created once and uses ES6 classes.

https://github.com/quinton-ashley/q5.js/blob/main/q5.js

GoToLoop commented 1 year ago

It doesn't seem correct to me! How is such an independent class now able to use Q5 instance functions like $.cos(), $.sin(), $.tan(), $.atan2()?

quinton-ashley commented 1 year ago

ah! good catch

quinton-ashley commented 1 year ago

Okay I think it's good now: https://github.com/quinton-ashley/q5.js/blob/39c3662c454519c932c29544384cd3bf42dd75f3/q5.js#L372

LingDong- / q5xjs

Bug in Vector.lerp() implementation #21