Consistency: Adding optional destination/output arguments to all applicable Vector functions

[Autsider666]

Context

I ran into a great presentation about increasing Vector math performance that advises to add output arguments to functions to prevent the creation of unneeded objects.

Proposal

EX.Vector already has some functions (scale, add, sub, rotate and clone) that have an optional dest argument, so I'm suggesting to consistently add an optional destargument to the following functions:

• min
• max
• normalize
• cross
• perpendicular

Optional

• Add an extra argument for the fallback anchor used in rotate
• Use destination or output instead of the abbreviated dest currently used.
• Update the documentation to inform developers of the potential performance gains from using this argument when using Vector. (Even more optional: Add code examples to show the differences)

[mattjennings]

Hm I didn't realize this was a thing. My personal thought on this is I don't like polluting all the methods with a param like this because it can already be chained onto any vector with the clone method

vec.rotate(...).clone(dest)

I would argue this should be the convention and instead remove any dest params - or, implement all these methods as a static that target vectors. (Then, the Vector instance methods simply call the static ones with itself as the target)

ex.Vector.rotate(vec, ...)

I think this method would help the most with performance issues as it lets you use all vector operations without instantiation

[Autsider666]

Hm I didn't realize this was a thing. My personal thought on this is I don't like polluting all the methods with a param like this because it can already be chained onto any vector with the clone method

vec.rotate(...).clone(dest)

Correct me if I'm wrong, but calling rotate without a dest argument would still create a new Vector object that would clutter up memory the moment it's returned (if called a lot of times every frame), even if you chain clone after it?

[mattjennings]

You're right, certain methods mutate itself while others create a new one and return it. IMO the consistency should be improved here too, with every operation mutating itself and instantiating new ones should be opt in with clone. This would be a big breaking change though.

[Autsider666]

I would argue this should be the convention and instead remove any dest params - or, implement all these methods as a static that target vectors. (Then, the Vector instance methods simply call the static ones with itself as the target)

ex.Vector.rotate(vec, ...)
I think this method would help the most with performance issues as it lets you use all vector operations without instantiation

TIL that JS/TS allows duplicate method definitions between static and instantiated methods....

I personally like the logic behind your suggestion now I know about it, but I'm not sure if it would be intuitive to understand without that prior knowledge.

[mattjennings]

I think it could be straight forward, and we could leave the current instance-method behaviour to avoid breaking changes (although I still think that should be given more thought). At least if you use the static methods, you know it's always mutating.

should roughly look like this in practice (just writing this out here, treat it as pseudocode):

class Vector {
   static rotate(target: Vector, angle: number, anchor?: Vector): Vector {
    const result = target
    if (!anchor) {
    // can be optimized, we dont need to instantiate another vec, we just need x/y
      anchor = new Vector(0, 0);
    }
    const sinAngle = Math.sin(angle);
    const cosAngle = Math.cos(angle);
    const x = cosAngle * (target.x - anchor.x) - sinAngle * (target.y - anchor.y) + anchor.x;
    const y = sinAngle * (target.x - anchor.x) + cosAngle * (target.y - anchor.y) + anchor.y;
    result.x = x;
    result.y = y;
    return result;
}

   rotate(angle: number, anchor?: Vector, dest?: Vector): Vector {
       return ex.Vector.rotate(dest ?? this, angle, anchor)
   }
}

[Autsider666]

I think I may have lost you, so just to check if we're talking about the same thing:

1. How would adding optional dest arguments to methods have the potential to create breaking changes?
2. What would be the added value of an extra static method if the instance-method still has the optional dest argument? (I hope I'm not nitpicking the pseudocode)
3. Does clone have a special meaning/function related to this topic or was it a random pick from the list of methods?

[mattjennings]

I'll try to summarize as I'm admittedly trying to expand the scope of this:

1. I propose against adding dest because it doesn't feel like the right solution. IMO the real problem is some vector methods mutate while others return a new one. I'd rather address that and make all methods mutate, and to get a new one you should always chain off of vec.clone().
2. Static methods would be half of my ideal solution for 1. without the breaking change. A static method requires a target, therefore always mutates. This can be the escape hatch for someone that needs it for performance, instead of adding dest to all the methods.

ex.Vector.rotate(vec, ...)

3. Clone returns either itself or clones to the target with clone(dest) (so it's more of a write in that scenario). To bring it back to the 1st point, it would be used to establish a cleaner API:

vec.rotate(...).scale(..) // mutates vec
vec.clone().rotate(..).scale(..) // returns a new mutated vec
vec.rotate(..).scale(..).clone(dest) // mutate vec and additionally write to dest

this gives flexibilty on how you want to use your vectors for old methods and new without the methods needing to be concerned about this.

[Autsider666]

Ah, that explains my confusion. My tired autistic brain was having issues last night connecting clone with my focus on using ex.Vector without creating any unnecessary new objects. (Preventing GC stutter feels like an impossible undertaking without it 😢 )

I now understand your drive to take the consistency of Vector even further, but I'm not yet sure if expanding the current scope would be preferable over creating a separate issue for it and handling this issue with smaller non-breaking changes. This is all based on not seeing any obvious interference between the two issues, but I'm not 100% sure I'm not overlooking anything.

[github-actions]

This issue hasn't had any recent activity lately and is being marked as stale automatically.