2019-10-19 11:56 EDT


View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0003134FSSCPgameplaypublic2015-05-10 08:36
ReporterAxem 
Assigned To 
PrioritynormalSeveritycrashReproducibilityalways
StatusresolvedResolutionfixed 
Product Version3.7.1 
Target Version3.7.4Fixed in Version3.7.3 
Summary0003134: dynamic weapon point firing causes a crash when used on a fighter with more than 6 firing points
DescriptionThis 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.
TagsNo tags attached.
Attached Files
  • patch file icon factor_table.patch (6,152 bytes) 2015-01-23 19:06 -
    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
    
    patch file icon factor_table.patch (6,152 bytes) 2015-01-23 19:06 +

-Relationships
+Relationships

-Notes

~0016414

MageKing17 (developer)

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.

~0016460

LotF (reporter)

Added a patch file that should solve this issue.

~0016465

chief1983 (administrator)

Thanks! We'll look into this soon.

~0016623

m_m (developer)

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.

~0016663

Goober5000 (administrator)

Tweaking target.

~0016692

chief1983 (administrator)

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.

~0016693

m_m (developer)

Yes, I can do that.

~0016698

chief1983 (administrator)

https://github.com/scp-fs2open/fs2open.github.com/pull/79

~0016701

niffiwan (developer)

fixed in aa6936a2a501ebea60366034afbe70a4c5f269ff

~0016702

niffiwan (developer)

bloody hell, what does it take to have this resolved without being assigned to me?

~0016703

chief1983 (administrator)

I gotcha man.
+Notes

-Issue History
Date Modified Username Field Change
2014-12-07 15:35 Axem New Issue
2014-12-07 15:41 MageKing17 Note Added: 0016414
2014-12-07 15:41 MageKing17 Assigned To => MageKing17
2014-12-07 15:41 MageKing17 Status new => assigned
2014-12-07 15:41 MageKing17 Target Version => 3.7.3
2015-01-23 19:06 LotF File Added: factor_table.patch
2015-01-23 19:07 LotF Note Added: 0016460
2015-01-23 21:13 MageKing17 Assigned To MageKing17 =>
2015-01-23 21:13 MageKing17 Status assigned => code review
2015-01-25 19:43 chief1983 Note Added: 0016465
2015-04-08 09:25 m_m Note Added: 0016623
2015-04-23 23:09 Goober5000 Note Added: 0016663
2015-04-23 23:09 Goober5000 Target Version 3.7.3 => 3.7.4
2015-05-04 15:39 chief1983 Note Added: 0016692
2015-05-04 15:42 m_m Note Added: 0016693
2015-05-07 13:57 chief1983 Note Added: 0016698
2015-05-10 07:37 niffiwan Note Added: 0016701
2015-05-10 07:37 niffiwan Status code review => resolved
2015-05-10 07:37 niffiwan Fixed in Version => 3.7.4
2015-05-10 07:37 niffiwan Resolution open => fixed
2015-05-10 07:37 niffiwan Assigned To => niffiwan
2015-05-10 07:37 niffiwan Assigned To niffiwan =>
2015-05-10 07:37 niffiwan Status resolved => feedback
2015-05-10 07:37 niffiwan Resolution fixed => reopened
2015-05-10 07:38 niffiwan Status feedback => resolved
2015-05-10 07:38 niffiwan Resolution reopened => fixed
2015-05-10 07:38 niffiwan Assigned To => niffiwan
2015-05-10 07:38 niffiwan Status resolved => feedback
2015-05-10 07:38 niffiwan Resolution fixed => reopened
2015-05-10 07:38 niffiwan Status feedback => resolved
2015-05-10 07:38 niffiwan Resolution reopened => fixed
2015-05-10 07:38 niffiwan Assigned To niffiwan =>
2015-05-10 07:38 niffiwan Status resolved => feedback
2015-05-10 07:38 niffiwan Resolution fixed => reopened
2015-05-10 07:39 niffiwan Note Added: 0016702
2015-05-10 07:39 niffiwan Status feedback => resolved
2015-05-10 07:39 niffiwan Resolution reopened => fixed
2015-05-10 07:39 niffiwan Assigned To => niffiwan
2015-05-10 08:36 chief1983 Note Added: 0016703
2015-05-10 08:36 chief1983 Assigned To niffiwan =>
2015-05-10 08:36 chief1983 Product Version => 3.7.1
2015-05-10 08:36 chief1983 Fixed in Version 3.7.4 => 3.7.3
+Issue History