View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0003127 | FSSCP | math-related | public | 2014-10-24 03:59 | 2014-11-10 01:56 |
| Reporter | MjnMixael | Assigned To | taylor | ||
| Priority | urgent | Severity | major | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Platform | PC | OS | Windows | OS Version | Win7 |
| Product Version | 3.7.2 | ||||
| Target Version | 3.7.2 | ||||
| Summary | 0003127: SVN Revision 11126 Causes Major Slowdown | ||||
| Description | SVN Revision 11126 causes major FPS loss, especially in dense missions that were previously playable. I noticed this in BtA, and tracked it to that revision. | ||||
| Steps To Reproduce | The issue was obvious, but I did not test it on retail. I don't know if any of those missions will display it. | ||||
| Tags | No tags attached. | ||||
|
|
The FPS loss is dependent on the number of objects within a certain proximity to each other. The first mission I noticed this in resulted in a drop to 40 FPS, the second one I tried had even more objects within a close (around 4 km radius) distance and resulted in a reduction to 6 FPS. To top it off, the problem only occurs when looking at or near the group of objects. |
|
|
fvi.cpp.patch (1,543 bytes)
Index: code/math/fvi.cpp
===================================================================
--- code/math/fvi.cpp (revision 11156)
+++ code/math/fvi.cpp (working copy)
@@ -325,7 +325,8 @@
*/
int fvi_ray_boundingbox( vec3d *min, vec3d *max, vec3d * p0, vec3d *pdir, vec3d *hitpt )
{
- int middle = ((1<<0) | (1<<1) | (1<<2));
+ bool inside = true;
+ bool middle[3] = { true, true, true };
int i;
int which_plane;
float maxt[3];
@@ -334,15 +335,17 @@
for (i = 0; i < 3; i++) {
if (p0->a1d[i] < min->a1d[i]) {
candidate_plane[i] = min->a1d[i];
- middle &= ~(1<<i);
+ middle[i] = false;
+ inside = false;
} else if (p0->a1d[i] > max->a1d[i]) {
candidate_plane[i] = max->a1d[i];
- middle &= ~(1<<i);
+ middle[i] = false;
+ inside = false;
}
}
// ray origin inside bounding box
- if (middle) {
+ if ( inside ) {
*hitpt = *p0;
return 1;
}
@@ -349,19 +352,17 @@
// calculate T distances to candidate plane
for (i = 0; i < 3; i++) {
- if ( (middle & (1<<i)) || (pdir->a1d[i] == 0.0f) ) {
+ if ( !middle[i] && (pdir->a1d[i] != 0.0f) )
+ maxt[i] = (candidate_plane[i] - p0->a1d[i]) / pdir->a1d[i];
+ else
maxt[i] = -1.0f;
- } else {
- maxt[i] = (candidate_plane[i] - p0->a1d[i]) / pdir->a1d[i];
- }
}
// Get largest of the maxt's for final choice of intersection
which_plane = 0;
for (i = 1; i < 3; i++) {
- if (maxt[which_plane] < maxt[i]) {
+ if (maxt[which_plane] < maxt[i])
which_plane = i;
- }
}
// check final candidate actually inside box
|
|
|
After DahBlount pointed me towards a mission demonstrating the problem, I narrowed the culprit down to the fvi_ray_boundingbox() changes. Reverting just those changes results in the performance drop disappearing. |
|
|
That makes sense, as those were actual algorithmic changes vs. the other function which contained optimizations but no changes to the algorithm. This ticket does surprise me though because the new algorithm uses bitwise operations which are usually faster than multiplication operations. It's possible that there's a mistake in the algorithm though. I'll ping Taylor. |
|
|
I don't see any multiplication operations being changed...? |
|
|
Mantis3127-fvi.cpp.patch (376 bytes)
Index: code/math/fvi.cpp
===================================================================
--- code/math/fvi.cpp (revision 11156)
+++ code/math/fvi.cpp (working copy)
@@ -341,8 +341,8 @@
}
}
- // ray origin inside bounding box
- if (middle) {
+ // ray origin inside bounding box?
+ if (middle & ((1<<0) | (1<<1) | (1<<2))) {
*hitpt = *p0;
return 1;
}
|
|
|
The multiplication is implicit. Array access is essentially pointer manipulation: multiply the array index by the element size, then add the result to the array base pointer. The new code treats "middle" as a bitfield rather than an array. Taylor spotted an unintended change which may be the cause of the problem. The if() block that tests "middle" used to check that all three of the axes were still inside the bounding box. The new code merely checks that any of the axes are still inside. I've attached a patch that should restore the original check. |
|
|
3127.patch (372 bytes)
Index: code/math/fvi.cpp
===================================================================
--- code/math/fvi.cpp (revision 11156)
+++ code/math/fvi.cpp (working copy)
@@ -341,8 +341,8 @@
}
}
- // ray origin inside bounding box
- if (middle) {
+ // ray origin inside bounding box?
+ if (middle == ((1<<0) | (1<<1) | (1<<2))) {
*hitpt = *p0;
return 1;
}
|
|
|
Your patch fails to actually remove the performance drop because it's effectively checking the same thing, in this context. Changing the "&" into an "==", however, gets rid of the performance drop by actually checking that all three bits are set. |
|
|
Derp, yeah, it *is* checking the same thing, isn't it. :p Glad to hear that's where the problem is, though. I'll commit the revised fix. |
|
|
Committed. If the next trunk build fixes the slowdown, I'll resolve the ticket. |
|
|
Problem solved? |
|
|
I haven't tested yet. |
|
|
Latest revision builds are back to normal speed. So I presume that this is fixed. |
|
|
Let's mark this as resolved, then. |
|
|
Crediting Taylor because it was he who figured out where the problem was. |
|
fs2open: trunk r11126 2014-10-12 16:14 Ported: N/A Details Diff |
xt branch: optimizations in fvi.cpp |
Affected Issues 0003127 |
|
| mod - /trunk/fs2_open/code/math/fvi.cpp | Diff File | ||
|
fs2open: trunk r11157 2014-10-25 22:43 Ported: N/A Details Diff |
Mantis 0003127: fix the spatial check in the rewritten algorithm |
Affected Issues 0003127 |
|
| mod - /trunk/fs2_open/code/math/fvi.cpp | Diff File | ||
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2014-10-24 03:59 | MjnMixael | New Issue | |
| 2014-10-24 19:25 | DahBlount | Note Added: 0016348 | |
| 2014-10-24 20:29 | Goober5000 | Changeset attached | => fs2open trunk r11126 |
| 2014-10-24 20:29 | Goober5000 | Assigned To | => Goober5000 |
| 2014-10-24 20:29 | Goober5000 | Status | new => assigned |
| 2014-10-24 22:45 | MageKing17 | File Added: fvi.cpp.patch | |
| 2014-10-24 22:46 | MageKing17 | Note Added: 0016351 | |
| 2014-10-25 02:22 | Goober5000 | Note Added: 0016352 | |
| 2014-10-25 03:16 | MageKing17 | Note Added: 0016354 | |
| 2014-10-25 21:43 | Goober5000 | File Added: Mantis3127-fvi.cpp.patch | |
| 2014-10-25 21:46 | Goober5000 | Note Added: 0016355 | |
| 2014-10-25 22:18 | MageKing17 | File Added: 3127.patch | |
| 2014-10-25 22:20 | MageKing17 | Note Added: 0016356 | |
| 2014-10-26 01:41 | Goober5000 | Note Added: 0016357 | |
| 2014-10-26 02:02 | Goober5000 | Changeset attached | => fs2open trunk r11157 |
| 2014-10-26 02:02 | Goober5000 | Note Added: 0016358 | |
| 2014-10-27 18:19 | chief1983 | Note Added: 0016361 | |
| 2014-10-28 05:15 | MjnMixael | Note Added: 0016362 | |
| 2014-10-31 04:56 | MjnMixael | Note Added: 0016364 | |
| 2014-10-31 05:54 | MageKing17 | Note Added: 0016365 | |
| 2014-10-31 05:54 | MageKing17 | Status | assigned => resolved |
| 2014-10-31 05:54 | MageKing17 | Resolution | open => fixed |
| 2014-11-10 01:56 | Goober5000 | Note Added: 0016381 | |
| 2014-11-10 01:56 | Goober5000 | Assigned To | Goober5000 => taylor |
