View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0003134 | FSSCP | gameplay | public | 2014-12-07 20:35 | 2015-05-10 12:36 |
| Reporter | Axem | Assigned To | |||
| Priority | normal | Severity | crash | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Product Version | 3.7.1 | ||||
| Target Version | 3.7.4 | Fixed in Version | 3.7.3 | ||
| Summary | 0003134: dynamic weapon point firing causes a crash when used on a fighter with more than 6 firing points | ||||
| Description | This is a bit different than the bug that causes a crash with exactly 6 firing points. MAX_NUM_SLOTS as defined in keycontrol.cpp is just 6, which seems really low considering we can have fighters with a lot more than 6 firing points in a bank. Any more than 6 and the game crashes and will trip an assert in debug. MageKing suggested to vectorize the table so we don't need arbitrary defines. | ||||
| Tags | No tags attached. | ||||
|
|
It should also be possible to figure out the size of the largest bank on a ship with dynamic fire linking, and combine that with using vectors, to minimize the memory footprint of the lookup table and avoid having to bump a #define in the future. If anybody else wants to tackle this problem before me, feel free. |
|
|
factor_table.patch (6,152 bytes)
Index: code/io/keycontrol.cpp
===================================================================
--- code/io/keycontrol.cpp (revision 11221)
+++ code/io/keycontrol.cpp (working copy)
@@ -16,6 +16,7 @@
#include "io/key.h"
#include "io/joy.h"
#include "io/timer.h"
+#include "math/factor_table.h"
#include "ship/ship.h"
#include "playerman/player.h"
#include "weapon/weapon.h"
@@ -60,80 +61,9 @@
#include "cmdline/cmdline.h"
#include "object/objectshield.h"
-#define MAX_NUM_SLOTS 6
-
-struct ftable
-{
-int count;
-int table[ MAX_NUM_SLOTS ];
-};
-
-class factor_table
- {
- public:
- factor_table();
- ~factor_table(){ delete[] table; }
- int getNextSlots( int slots_on_ship, int cur_slots );
-
- private:
- ftable * table;
- };
-
+// Natural number factor lookup table
factor_table ftables;
-static bool isAPrimeFactor( int factor, int product )
-{
-return ( (float)product / (float)factor ) == (product / factor);
-}
-
-factor_table::factor_table()
-{
-table = new ftable[ MAX_NUM_SLOTS ];
-
-memset( table, 0x00, sizeof( ftable ) * MAX_NUM_SLOTS );
-for( int i = 0 ; i < MAX_NUM_SLOTS; ++i )
- {
- table[ i ].count = 0;
- for( int j = 1; j <= i + 1; ++j )
- {
- if( isAPrimeFactor( j, i + 1 ) )
- {
- table[ i ].table[ table[ i ].count ] = j;
- table[ i ].count ++;
- }
- }
- }
-}
-
-int factor_table::getNextSlots(int slots_on_ship, int cur_slots)
-{
-Assertion( slots_on_ship <= MAX_NUM_SLOTS, "factor_table::getNextSlots() called with %d slots, when MAX_NUM_SLOTS is %d; get a coder!\n", slots_on_ship, MAX_NUM_SLOTS );
-Assertion( slots_on_ship >= 1, "factor_table::getNextSlots() called with %d slots, when only positive integers make sense; get a coder!\n" );
-
-int slots_index = slots_on_ship - 1;
-
-for( int i = 0; i < table[ slots_index ].count; ++i )
- {
- if( table[ slots_index ].table[ i ] == cur_slots )
- {
- if( table[ slots_index ].count == i + 1 )
- {
- //Overflow back to 1
- return 1;
- }
- else
- {
- //Next block in the table
- return table[ slots_index ].table[ i + 1 ];
- }
- }
- }
-//Did not find cur_slots, try and get back on track
-
-Assertion( false, "For some reason, factor_table::getNextSlots() was unable to locate cur_slots. This should never happen; get a coder!\n" );
-return 1;
-}
-
// --------------------------------------------------------------
// Global to file
// --------------------------------------------------------------
@@ -1739,7 +1669,7 @@
ship_info *sip = &Ship_info[shipp->ship_info_index];
if (sip->flags2 & SIF2_DYN_PRIMARY_LINKING) {
polymodel *pm = model_get( sip->model_num );
- count = ftables.getNextSlots( pm->gun_banks[ swp->current_primary_bank ].num_slots, swp->primary_bank_slot_count[ swp->current_primary_bank ] );
+ count = ftables.getNext( pm->gun_banks[ swp->current_primary_bank ].num_slots, swp->primary_bank_slot_count[ swp->current_primary_bank ] );
swp->primary_bank_slot_count[ swp->current_primary_bank ] = count;
shipp->last_fired_point[ swp->current_primary_bank ] += count - ( shipp->last_fired_point[ swp->current_primary_bank ] % count);
shipp->last_fired_point[ swp->current_primary_bank ] -= 1;
Index: code/math/factor_table.cpp
===================================================================
--- code/math/factor_table.cpp (revision 0)
+++ code/math/factor_table.cpp (working copy)
@@ -0,0 +1,63 @@
+
+#include "globalincs/pstypes.h"
+#include "math/factor_table.h"
+
+factor_table::factor_table(size_t size)
+{
+ resize(size);
+}
+
+factor_table::~factor_table() {}
+
+size_t factor_table::getNext(size_t n, size_t current)
+{
+ Assertion(n >= 1, "factor_table::getNext() called with %d, when only natural numbers make sense; get a coder!\n");
+
+ // Resize lookup table if the value is greater than the current size
+ if (n > _lookup.size())
+ resize(n);
+
+ int index = n - 1;
+ for (size_t i = 0; i < _lookup[index].size(); ++i)
+ {
+ if (_lookup[index][i] == current)
+ {
+ if (_lookup[index].size() == i + 1)
+ {
+ // Overflow back to 1
+ return 1;
+ }
+ else
+ {
+ // Next factor in the table
+ return _lookup[index][i + 1];
+ }
+ }
+ }
+
+ Assertion( false, "For some reason, factor_table::getNext() was unable to locate the current factor. This should never happen; get a coder!\n" );
+ return 1;
+}
+
+void factor_table::resize(size_t size)
+{
+ size_t oldSize = _lookup.size();
+ _lookup.resize(size);
+
+ // Fill lookup table for the missing values
+ for (size_t i = oldSize; i < size; ++i)
+ {
+ for (size_t j = 1; j <= i + 1; ++j)
+ {
+ if (isNaturalNumberFactor(j, i + 1))
+ {
+ _lookup[i].push_back(j);
+ }
+ }
+ }
+}
+
+bool factor_table::isNaturalNumberFactor(size_t factor, size_t n)
+{
+ return ((float) n / (float) factor) == n / factor;
+}
Index: code/math/factor_table.h
===================================================================
--- code/math/factor_table.h (revision 0)
+++ code/math/factor_table.h (working copy)
@@ -0,0 +1,54 @@
+
+#ifndef __FACTOR_TABLE_H__
+#define __FACTOR_TABLE_H__
+
+#include "globalincs/vmallocator.h"
+
+/**
+ * Natural number factor lookup class.
+ */
+class factor_table
+{
+ public:
+ /**
+ * Constructor.
+ *
+ * @param size The desired initial size of the lookup table.
+ */
+ factor_table(size_t size = 6);
+
+ /**
+ * Destructor.
+ */
+ ~factor_table();
+
+ /**
+ * Returns the next natural number factor for the given number and current factor.
+ *
+ * @param maximum The number to lookup the next natural number factor for.
+ * @param current The current natural number factor.
+ * @return A natural number factor.
+ */
+ size_t getNext(size_t n, size_t current);
+
+ private:
+
+ SCP_vector< SCP_vector<size_t> > _lookup;
+
+ /**
+ * Grow the internal lookup table based on the desired size.
+ *
+ * @param size The desired size.
+ */
+ void resize(size_t size);
+
+ /**
+ * Returns true if the given factor is a natural number factor for the given value.
+ *
+ * @param factor The factor.
+ * @param n The value.
+ */
+ static bool isNaturalNumberFactor(size_t factor, size_t n);
+};
+
+#endif
|
|
|
Added a patch file that should solve this issue. |
|
|
Thanks! We'll look into this soon. |
|
|
Minor things I noticed: - factortable.cpp Line 14: The parameter for %d (presumable n) is missing Line: 60: isNaturalNumberFactor doesn't need to be a static member of the class. You could use a static function or wrap the definition in an anonymous namespace. |
|
|
Tweaking target. |
|
|
m!m, any chance you'd want to clean this up and put it in a pull request on his behalf? He hasn't responded yet. |
|
|
Yes, I can do that. |
|
|
https://github.com/scp-fs2open/fs2open.github.com/pull/79 |
|
|
fixed in aa6936a2a501ebea60366034afbe70a4c5f269ff |
|
|
bloody hell, what does it take to have this resolved without being assigned to me? |
|
|
I gotcha man. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2014-12-07 20:35 | Axem | New Issue | |
| 2014-12-07 20:41 | MageKing17 | Note Added: 0016414 | |
| 2014-12-07 20:41 | MageKing17 | Assigned To | => MageKing17 |
| 2014-12-07 20:41 | MageKing17 | Status | new => assigned |
| 2014-12-07 20:41 | MageKing17 | Target Version | => 3.7.3 |
| 2015-01-24 00:06 | LotF | File Added: factor_table.patch | |
| 2015-01-24 00:07 | LotF | Note Added: 0016460 | |
| 2015-01-24 02:13 | MageKing17 | Assigned To | MageKing17 => |
| 2015-01-24 02:13 | MageKing17 | Status | assigned => code review |
| 2015-01-26 00:43 | chief1983 | Note Added: 0016465 | |
| 2015-04-08 13:25 | m_m | Note Added: 0016623 | |
| 2015-04-24 03:09 | Goober5000 | Note Added: 0016663 | |
| 2015-04-24 03:09 | Goober5000 | Target Version | 3.7.3 => 3.7.4 |
| 2015-05-04 19:39 | chief1983 | Note Added: 0016692 | |
| 2015-05-04 19:42 | m_m | Note Added: 0016693 | |
| 2015-05-07 17:57 | chief1983 | Note Added: 0016698 | |
| 2015-05-10 11:37 | niffiwan | Note Added: 0016701 | |
| 2015-05-10 11:37 | niffiwan | Status | code review => resolved |
| 2015-05-10 11:37 | niffiwan | Fixed in Version | => 3.7.4 |
| 2015-05-10 11:37 | niffiwan | Resolution | open => fixed |
| 2015-05-10 11:37 | niffiwan | Assigned To | => niffiwan |
| 2015-05-10 11:37 | niffiwan | Assigned To | niffiwan => |
| 2015-05-10 11:37 | niffiwan | Status | resolved => feedback |
| 2015-05-10 11:37 | niffiwan | Resolution | fixed => reopened |
| 2015-05-10 11:38 | niffiwan | Status | feedback => resolved |
| 2015-05-10 11:38 | niffiwan | Resolution | reopened => fixed |
| 2015-05-10 11:38 | niffiwan | Assigned To | => niffiwan |
| 2015-05-10 11:38 | niffiwan | Status | resolved => feedback |
| 2015-05-10 11:38 | niffiwan | Resolution | fixed => reopened |
| 2015-05-10 11:38 | niffiwan | Status | feedback => resolved |
| 2015-05-10 11:38 | niffiwan | Resolution | reopened => fixed |
| 2015-05-10 11:38 | niffiwan | Assigned To | niffiwan => |
| 2015-05-10 11:38 | niffiwan | Status | resolved => feedback |
| 2015-05-10 11:38 | niffiwan | Resolution | fixed => reopened |
| 2015-05-10 11:39 | niffiwan | Note Added: 0016702 | |
| 2015-05-10 11:39 | niffiwan | Status | feedback => resolved |
| 2015-05-10 11:39 | niffiwan | Resolution | reopened => fixed |
| 2015-05-10 11:39 | niffiwan | Assigned To | => niffiwan |
| 2015-05-10 12:36 | chief1983 | Note Added: 0016703 | |
| 2015-05-10 12:36 | chief1983 | Assigned To | niffiwan => |
| 2015-05-10 12:36 | chief1983 | Product Version | => 3.7.1 |
| 2015-05-10 12:36 | chief1983 | Fixed in Version | 3.7.4 => 3.7.3 |