mat4_mul_mvp

[Minterl]

Addresses #124 I understand that this would just be another layer of abstraction and not an actual optimized algorithm, but the use case is common enough (given we do have functions for constructing rotation, perspective projection, etc. matrices) that it would make sense to have an official idiom for constructing an MVP matrix.

I am haven't tested this, but as for implementing an optimized, explicit three matrix multiplication algorithm, I would wager that the the inputs are generally small enough that something like parallel Strassen is generally unnecessary.

[recp]

Hi @Minterl,

Many thanks for your contributions,

Let’s get some feedback on naming and parameter order, before proceeding, then we can merge it 🚀

[lczyk]

• I'm unsure about calling it mvp. 1) it's a specific naming convention while the function is doing something a bit more general. 2) the 'v' in 'mvp' suggests a vector input (or is it just me?). I'd suggest glm_mat4_mul3 ?? Just a thought though. No hard feelings on this

• missing glms_mat4_mul_mvp

• missing test entries for glm_mat4_mul_mvp

• missgin docs for glm_mat4_mul_mvp

^ found through cheeky rg '_mat4_mulN'

@Minterl Re missing bits, they were all small so i've made a PR to your branch from MarcinKonowalczyk/cglm/tree/glm_mat4_mul3, if you'd like. :)) Feel free to use or ignore.

[Minterl]

@MarcinKonowalczyk Thanks for the PR; I've merged it. That said, I think my PR needs a little more work before it's ready to be merged. I agree that glm_mat4_mul_mvp might not be the absolute best name, especially if you consider that glm_mat4_mul3 lines up with other names in cglm so well. It pigeonholes what otherwise might be a versatile function into what might turn into unclear code when used outside of the context of MVP multiplication. One of the main reasons I made the PR in the first place was so that I could have total confidence in the multiplication order without having to troubleshoot and write test cases for something that should be handled on the library end. In my eyes, the best compromise is to have a well-written example in the docs. I'll make the changes described here when I get the chance.

[appybara13]

glm_mul already exists for multiplying two affine matrices (a model and view matrix). If mat4_mul_mvp is intended just for MVP, it should call that function.

If the name is changed to something more general, it will be slower for MVP multiplication.

[lczyk]

glm_mul already exists for multiplying two affine matrices...

There was no suggestion of naming anything here glm_mul, but just giving the function a bit more general name: glm_mat4_mul3 instead of glm_mat4_mul_mvp. Sorry if i misunderstood your comment though.

If the name is changed to something more general, it will be slower for MVP multiplication.

I'm unsure what you mean? How would the name of the function affect its performance? Surely the idea is to make it as performant as possible. I can't think of MVP-specific optimisation which would not be applicable to just multiplying 3 matrices together.

[Minterl]

If the name is changed to something more general, it will be slower for MVP multiplication.

What exactly do you mean by this? mat4_mul isn't called mat4_mul_v_p because it's useful in other contexts. mat4_mul3 could be used, for instance, to construct a skinning matrix with a model transformation multiplied in.

[appybara13]

If the name is changed to something more general, it will be slower for MVP multiplication.

What exactly do you mean by this? mat4_mul isn't called mat4_mul_v_p because it's useful in other contexts. mat4_mul3 could be used, for instance, to construct a skinning matrix with a model transformation multiplied in.

Sorry for not being clear.

I mean that, in the specific case where you are doing an MVP multiplication, you wouldn't want to use the function in this PR because it is inefficient compared to the following:

glm_mul(view, model, mv)
glm_mat4_mul(proj, mv, mvp)

This is discussed in #124.

So it probably makes sense to add both glm_mat4_mul3 and a glm_mat4_mvp, or to add neither. My view is that merging this PR as-is will encourage sub-optimal MVP multiplication.

[appybara13]

I also think that using (right, center, left, out) as a parameter order for glm_mat4_mul3 is inconsistent with the parameter order (left, right, out) for glm_mat4.

[recp]

+1 for @appybara13 proposal:

glm_mat4_mvp {
  glm_mul(view, model, mv)
  glm_mat4_mul(proj, mv, mvp)
}
/* 2 spaces for indents :) */

seems good to me,

I think we can introduce a few functions

• glm_mat4_mul3x(m1, m2, m3, dest) general purpose 3 matrix multiplication
• glm_mat4_mul4x(m1, m2, m3, m4, dest) general purpose 4 matrix multiplication
• glm_mat4_mvp() MVP multiplication ( not sure param order m, v, p vs p, v, m, it may be good to follow naming ).

I think no need to add mul explicitly to glms_mat4_mul_mvp since MVP indicates a multiplication already. x like _mul4x suffix is good addition to separate things like glm_mat4_mulv3, glm_mat4_pick3... I mean glm_mat4_mul3 may be confused with 'multiply mat4 with mat3'. Or even preserving the glm_mat4_mul3 for future opens a room to promote mat3 and multiply with mat4 for instance

camera intrinsics matrix ( mat3 ) x camera world / extrinsic matrix ( mat4 ) x projection ( mat4 ) ...

[lczyk]

...x like _mul4x suffix is good addition to separate things

agreed

because it is inefficient compared to the following: ...

yup (i missed this from the original discussion 😅🫣)

[recp]

@Minterl ping.