gl-matrix icon indicating copy to clipboard operation
gl-matrix copied to clipboard

Class and chaining

Open Rycochet opened this issue 6 years ago • 17 comments

I'm a fan of chaining calls rather than passing data, and wondering if it'd be worthwhile adding classes to this to allow chaining on top of the current way. Not thinking about duplicating code, but just acting as an interface to the functional code.

export class Mat4 extends Float32Array {
    constructor() {
        super(16);
        this.identity();
    }

    identity = () => mat4.identity(this);

    rotate = (rad, axis) => return mat4.rotate(this, this, rad, axis);

    // etc
}

Obviously it's Float32Array only, but it should be pretty quick to add, the only downside is that the documentation would need to be duplicated for pretty IDE environments. The constructor could also possibly take an optional argument for cloning from (if it checks type then an array length of 16 could be done etc).

The result would be something like this -

/// OLD
var transformMatrix = mat4.create();
mat4.fromTranslation(transformMatrix, [translateX, translateY, 0]);
mat4.scale(transformMatrix, transformMatrix, [scale, -scale, scale])

// NEW
var transformMatrix = new Mat4()
        .fromTranslation([translateX, translateY, 0])
        .scale([scale, -scale, scale]);

Rycochet avatar May 11 '18 11:05 Rycochet

That looks awesome! 😄

acting as an interface to the functional code

So, it would be slower, but neater. And, if the user wants speed, he can always use the functional code. I like it.

Now the only question is: Do the benefits outweigh the maintenance effort.

stefnotch avatar May 11 '18 12:05 stefnotch

I'm not sure how much slower it might be - modern browsers are pretty good with the caching of values and as that's a direct through call with only a couple of extra arguments (that are already in the cache) being pushed in front I'd expect it to be within 0.1% speed - but would love to see some benchmarks if it's there ;-)

I'm happy to add a PR when I have time - but stupidly busy in general so that might not be for a while. The tests can highlight when there's a mismatch between the class and the module - so hopefully there's not much maintenance to be had.

Rycochet avatar May 11 '18 12:05 Rycochet

@Rycochet Do you think that the majority of that could easily be auto-generated? 🤔

stefnotch avatar May 14 '18 15:05 stefnotch

@stefnotch Actually - yes - since it's pretty much a complete copy of any exported functions with a very limited pattern match on it - anything that doesn't match the pattern would be something that should be ignored - wouldn't even need tokenizing, simply a regex pattern...

Rycochet avatar May 14 '18 17:05 Rycochet

@Rycochet So, I have an excuse to use regex? Or is there a "better" way to do so?

@maierfelix What were you using in your WebAssembly port experiments?

I did some experiments and by parsing each module's comments and receiving the type information from there.

stefnotch avatar May 14 '18 17:05 stefnotch

As @stefnotch mention above, I think it will be slower, which might be a problem since most of us use this library for 3D and the faster the library performs the faster our 3D application will be.

Also, we're creating pointers every time we need to call a new Mat4() which might have an impact on Garbage Collection..

Maybe its worth doing a couple of tests before moving forward?

andrevenancio avatar May 17 '18 16:05 andrevenancio

@stefnotch Easiest is probably regex simply because it's a nice and simple pattern to match - doesn't have to be pretty if it's only pre-processing - could do some tokenizing, but would need someone with more time to design, even though it'd be a far better solution in the long run.

As a subclass of Array, there's almost no difference at all in terms of speed for creation (assuming that people were comparing to *.create()) - there's a lot of things that are good and bad for speed, and moving over to a class based pattern will only make a difference to people who don't understand how it's doing things to begin with. Technically by forcing people to use new they're more likely to realise it's there (instead of not realising it's hidden behind *.create()).

Having one extra step in the indirect calling of methods makes no real difference due to the fact that it's working on this, which would be staying in context - and chaining makes for easier optimisations for the browser.

So basically - we need something to test against, then we need tests, then see if it's an almost unmeasurable difference, or if it's significant enough that it shouldn't be followed through on ;-)

Rycochet avatar May 17 '18 16:05 Rycochet

@toji wrote a post a couple of years ago when he launched gl-matrix and he explained his thoughts behind it.

andrevenancio avatar May 17 '18 16:05 andrevenancio

Some quick benchmarks: https://jsbench.github.io/#1aa0ac752bc15eca5b53ad313e1f610a

stefnotch avatar May 17 '18 16:05 stefnotch

This could probably be developed as a standalone project. (A simple wrapper around gl-matrix)

stefnotch avatar May 17 '18 17:05 stefnotch

I pulled together a quick commentless (and untested) wrapper the way it could probably work. I've put some optional args in where it's sort of logical (so you can do two matrices into this one) - but as it's still a valid target for the normal function calls I'm not sure if it's needed. My one comment on using arrow functions is that when they're supported they're lower cost than normal functions, sadly us developers still have to support IE11, which doesn't support either of those...

import {
	add,
	adjoint,
	copy,
	determinant,
	equals,
	exactEquals,
	frob,
	fromQuat,
	fromQuat2,
	fromRotation,
	fromRotationTranslation,
	fromRotationTranslationScale,
	fromRotationTranslationScaleOrigin,
	fromScaling,
	fromTranslation,
	fromXRotation,
	fromYRotation,
	fromZRotation,
	frustum,
	getRotation,
	getScaling,
	getTranslation,
	identity,
	invert,
	lookAt,
	multiply,
	multiplyScalar,
	multiplyScalarAndAdd,
	ortho,
	perspective,
	perspectiveFromFieldOfView,
	rotate,
	rotateX,
	rotateY,
	rotateZ,
	scale,
	set,
	str,
	subtract,
	targetTo,
	translate,
	transpose,
} from "./Mat4.js";

// .create -> constructor
// .clone -> constructor optional arg
// .fromValues -> constructor optional args
// .str -> toString

export class Mat4 extends Float32Array {
	constructor(a) {
		super(16);
		if (arguments.length === 16) {}
			this.set.apply(this, arguments);
		} else if (a) {
			this.copy(a);
		} else {
			this.identity();
		}
	}

	add => (a, b) => add(this, b ? a : this, b || a);
	adjoint => (a) => adjoint(this, a);
	copy = (a) => copy(this, a);
	determinant = () => determinant(this);
	equals = (a) => equals(this, a);
	exactEquals = (a) => exactEquals(this, a);
	frob = () => frob(this);
	fromQuat = (q) => fromQuat(this, q);
	fromQuat2 = (a) => fromQuat2(this, a);
	fromRotation = (rad, axis) => fromRotation(this, rad, axis);
	fromRotationTranslation = (q, v) => fromRotationTranslation(this, z, v);
	fromRotationTranslationScale = (q, v, s) => fromRotationTranslationScale(this, q, v, s);
	fromRotationTranslationScaleOrigin = (q, v, s, o) => fromRotationTranslationScaleOrigin(this, q, v, s, o);
	fromScaling = (v) => fromScaling(this, v);
	fromTranslation = (v) => fromTranslation(this, v);
	fromXRotation = (rad, axis) => fromXRotation(this, rad, axis);
	fromYRotation = (rad, axis) => fromYRotation(this, rad, axis);
	fromZRotation = (rad, axis) => fromZRotation(this, rad, axis);
	frustum = (left, right, bottom, top, near, far) => frustum(this, left, right, bottom, top, near, far);
	getRotation = (mat) => getRotation(this, mat);
	getScaling = (mat) => getScaling(this, mat);
	getTranslation = (mat) => getTranslation(this, mat);
	identity = () => identity(this);
	invert = (a) => invert(this, a);
	lookAt = (eye, center, up) => lookAt(this, eye, center, up);
	multiply = (a, b) => multiply(this, b ? a : this, b || a);
	multiplyScalar => (a, b) => multiplyScalar(this, b ? a : this, b || a);
	multiplyScalarAndAdd => (a, scale, b) => multiplyScalarAndAdd(this, b ? a : this, b || a, scale);
	ortho = (left, right, bottom, top, near, far) => ortho(this, left, right, bottom, top, near, far);
	perspective = (fovy, aspect, near, far) => perspective(this, fovy, aspect, near, far);
	perspectiveFromFieldOfView = (fov, near, far) => perspectiveFromFieldOfView(this, fov, near, far);
	rotate = (rad, axis, a) => rotate(this, a || this, rad, axis);
	rotateX = (rad, a) => rotateX(this, a || this, rad);
	rotateY = (rad, a) => rotateY(this, a || this, rad);
	rotateZ = (rad, a) => rotateZ(this, a || this, rad);
	scale => (v, a) => scale(this, a || this, v);
	set = (m00, m01, m02, m03, m10, m11, m12, m13, m20, m21, m22, m23, m30, m31, m32, m33) => set(this, m00, m01, m02, m03, m10, m11, m12, m13, m20, m21, m22, m23, m30, m31, m32, m33);
	subtract => (a, b) => subtract(this, b ? a : this, b || a);
	targetTo = (eye, target, up) => targetTo(this, eye, target, up);
	toString = () => str(this);
	translate = (v, a) => translate(this, a || this, v);
	transpose = (a) => transpose(this, a);
}

Rycochet avatar May 17 '18 17:05 Rycochet

Don't totally hate the idea, there's a few things I think would be useful and we could get them if we approach it this way.

accessing x, y, z props instead of [0], [1], [2] can be less alien to some people.. and we could also easily create methods like we have in GLSL like vector.xyz, or vector.zyx etc...

But again, I think the readability will be better and the performance wont..

So for example: Something like

updateMatrices() {
    mat4.identity(matrices.modelView);
    mat4.copy(matrices.modelView, matrix);
    mat4.invert(matrices.inversedModelView, matrices.modelView);
    mat4.transpose(matrices.inversedModelView, matrices.inversedModelView);
    mat4.identity(matrices.normal);
    mat4.copy(matrices.normal, matrices.inversedModelView);
}

will be

updateMatrices(matrix) {
matrices.modelView
    .identity()
    .copy(matrix);

matrices.inversedModelView
    .invert()
    .transpose(matrices.inversedModelView);

matrices.normal
    .identity()
    .copy(matrices.inversedModelView);
}

?

andrevenancio avatar May 17 '18 17:05 andrevenancio

I'm pretty much entirely Typescript, so been using accessors for a while now - and adding getters and setters would be easy - however (and it's a big one) - using them is seriously bad performance-wise, even if it is a lot easier to read (in some instances). Saying that, because of the way they work, it would be cacheable by the browser - I'd expect something like a 20% performance drop using them (but again, performance tests). They would allow for some of the nifty things you see from GLSL though, and maybe that would be useful (test things in JS before moving it into GLSL where you lose the easy debugging etc).

Rycochet avatar May 17 '18 18:05 Rycochet

Relevant issue https://github.com/toji/gl-matrix/issues/276

stefnotch avatar May 17 '18 20:05 stefnotch

@Rycochet I just benchmarked getters, and it turns out that your guess is pretty much spot on. There is quite a performance penalty for using them.

stefnotch avatar May 18 '18 08:05 stefnotch

If you just wrap the Float32Array it's not nearly as slow as extending it. https://jsbench.github.io/#8fe2a35023c95c3ade4e64885bf1c5a7

tekgoblin avatar Nov 30 '20 13:11 tekgoblin

Another interesting library in that regard is https://github.com/matthiasferch/tsm

stefnotch avatar Aug 06 '22 11:08 stefnotch