I've noticed that RotatePointAroundVector() has changed over time (or Tremulous has its own version). The latest one in Q3 is longer, reuses other routines, and runs much more slower (2x on my compiler). WTF is up with that? I know readability is also important, but not for math routines. Besides, it's not any more readable than the older versions. So here's my version, it beats others by code size, readability and most importantly, performance. Then I've also added a tweak to AngleVectors().
1. Tremulous has its own version. The Q3 one hasn't changed.
2. Please submit unified diffs, not code fragments.
3. Please conduct yourself more professionally. This is a bug reporting system, not a web forum.
I learn from my mistakes...
Anyways, improvements to the game engine should go to the root, Quake 3. If Tremulous has better code, Q3 should "borrow" it. So are you going to commit my or Tremulous' code (after testing it) or not?
A performance comparison between all three would help, as would some correctness verification. I don't want to be making fundamental changes to math functions without thorough proof it makes sense. One thing you have to bear in mind is that existing Q3 mods are all using the existing RotatePointAroundVector, so you don't want the resultants differing, even if it's only by a small quantity as that can cause prediction misses and other weirdness.
Created attachment 1486[details]
RotatePointAroundVector() accuracy test and performance test
This is the program I used to test the function, now I've included all three versions in it for comparison.
On my compiler, using float, and fast fp model, the functions do differ in accuracy a bit: they're accurate to ~6 digits, so basically I think they differ only as much as the float's epsilon. Performance run: Q3=23373ms, TREM=11186ms, NEW=10235ms(10% faster).
OK, so if this will cause prediction misses with higher values, then maybe set this to REMIND, and add it in an ioq3 point release.
Created attachment 1533[details]
Unified Diff for Tweaks
Here is a unified diff for the changes, against revision 972.
I am testing them at the moment.
(In reply to comment #8)
> Created an attachment (id=1534) [details]
> Revised Unified Patch
>
> Ooops. Accidentally reversed the arguments to diff. Here's the fixed patch. xD
>
Tested it somewhat and it seems to have no problems I can detect.I do note however that my fps at the the human base on Transit increased slightly. (5-10fps) I am unsure if this is due to the patch, or perhaps my computer is performing better than usual.
(In reply to comment #9)
Your computer is performing fast whatsoever. RPAV() function runs 10% faster, the other, AV() runs 50% faster in a certain case. But Tremulous isn't based on RPAV() and AV() functions, is it? ;) I wouldn't expect more than +1FPS.
Comment 11/dev/humancontroller
2007-11-05 14:22:53 EST
Let's remove the staticness of the cp, cy, etc. variables, because (1) the ms compiler info is outdated, and (2) the project isn't compiled with the ms compiler anyway.
Created attachment 1818[details]
ProjectPointOnPlane()
Allow me to spam you with more tweaks.
It seems to me that this function doesn't behave correctly when the normal vector isn't normalized, though the code only passes normalized ones (that should be the correct use, am I right?).
As with the other patches, if this won't make it into ioQuake3, it could be applied to Tremulous or others.
PS: There's a difference between ioQuake3 and Tremulous versions, and the latter's version is weirder.
> though the code only passes normalized ones
I would add assert to emphasize that normal in ProjectPointOnPlane() and dir in RotatePointAroundVector() must have 1±ε length.
Created attachment 1481 [details] RotatePointAroundVector()Created attachment 1482 [details] AngleVectors()Created attachment 1486 [details] RotatePointAroundVector() accuracy test and performance test This is the program I used to test the function, now I've included all three versions in it for comparison. On my compiler, using float, and fast fp model, the functions do differ in accuracy a bit: they're accurate to ~6 digits, so basically I think they differ only as much as the float's epsilon. Performance run: Q3=23373ms, TREM=11186ms, NEW=10235ms(10% faster). OK, so if this will cause prediction misses with higher values, then maybe set this to REMIND, and add it in an ioq3 point release.Created attachment 1533 [details] Unified Diff for Tweaks Here is a unified diff for the changes, against revision 972. I am testing them at the moment.Created attachment 1534 [details] Revised Unified Patch Ooops. Accidentally reversed the arguments to diff. Here's the fixed patch. xDCreated attachment 1818 [details] ProjectPointOnPlane() Allow me to spam you with more tweaks. It seems to me that this function doesn't behave correctly when the normal vector isn't normalized, though the code only passes normalized ones (that should be the correct use, am I right?). As with the other patches, if this won't make it into ioQuake3, it could be applied to Tremulous or others. PS: There's a difference between ioQuake3 and Tremulous versions, and the latter's version is weirder.> though the code only passes normalized ones I would add assert to emphasize that normal in ProjectPointOnPlane() and dir in RotatePointAroundVector() must have 1±ε length.