Skip to content
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

Some changes, additions #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Some changes, additions #6

wants to merge 2 commits into from

Conversation

Aransentin
Copy link
Contributor

Hi there! I made some changes to your library:

Functionality:

  • Added vec3_reflect() and vec4_reflect().
  • Interchanged the order of all loops so that memory is accessed serially, like it was in mat4x4_from_vec3_mul_outer()
  • Rewrote mat4x4_invert to one using Laplace expansion instead; the previous one didn't seem to work at all, returning wrong data. The new function also uses 96 multiplications as opposed to the previous one using 160.
  • Removed all the memcpy() functions, and instead write directly to the output. You might want to revert this if you had some reason doing that.

Style:

  • Fixed spacing everywhere, eg. (v,v) to (v, v)
  • Removed braces around single statements, as it was inconsistently applied.
  • Added ".f" to all values, so that they are explicitly floats instead of doubles. Also unified style, eg "0.0f" to "0.f"

@datenwolf
Copy link
Owner

Wow, that's quite some work. However you'll have to add the memcpy stuff again, because by removing it you broke "in-place" operations. Think like

mat4x4 M;
/* do some stuff with M */
mat4x4_mul(M, M, T);
/* so more stuff with M */
mat4x4_transpose(M, M);

are broken now. It's most easy to see for transpose, which is essentially a swap across the diagonal. When writing a swap you must do

temp = a;
a = b;
b = a;

By removing the memcpy, e.g. in transpose you're clobbering the values you're later going to need in that loop.

In invert, Laplace expansion is nice. I did that determinant thing, by having it written out by some code generator (actually I was mostly interested in what a compiler's optimizer would make out of it). However I might replace it with a decomposition based method; using a eigendecomposition based method would give you eigenvalue/eigenvectors code, which again can be used for quaternion from rotation matrix calculations.

@Aransentin
Copy link
Contributor Author

You are correct about the temporary memory buffers, of course... I re-added them.
I also changed them around a little:

  • sizeof() now always takes the source variable (stops gcc -pedantic from complaining, mostly)
  • All temporary variables are now named x_ , where x is the name of the variable it takes the place of (some functions where written like that before, so I just implemented it everywhere)

@tdryer
Copy link

tdryer commented Dec 21, 2013

Looks like @Aransentin's first commit was merged but not the second, so the temporary buffers are currently missing in master and in-place operations are broken.

@Shourn
Copy link

Shourn commented Aug 20, 2015

just poking at this, because nearly 2 years later in-place operations are still broken :(
would be very nice if @Aransentin second commit could be merged sometime soon

@dizcza
Copy link
Contributor

dizcza commented Mar 27, 2017

Do we really need memcpy? We could use vec##n##_copy and mem4x4_dup instead and let the compiler optimize those 2d-loops.
I wonder which would be better on ARM Cortex-M4 in different scenarios: with flags -O0 up to -O3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants