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 |