View Issue Details

IDProjectCategoryView StatusLast Update
0003134FSSCPgameplaypublic2015-05-10 12:36
ReporterAxem Assigned To 
PrioritynormalSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
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.

Activities

MageKing17

2014-12-07 20:41

developer   ~0016414

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.

LotF

2015-01-24 00:06

reporter  

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
factor_table.patch (6,152 bytes)   

LotF

2015-01-24 00:07

reporter   ~0016460

Added a patch file that should solve this issue.

chief1983

2015-01-26 00:43

administrator   ~0016465

Thanks! We'll look into this soon.

m_m

2015-04-08 13:25

developer   ~0016623

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.

Goober5000

2015-04-24 03:09

administrator   ~0016663

Tweaking target.

chief1983

2015-05-04 19:39

administrator   ~0016692

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.

m_m

2015-05-04 19:42

developer   ~0016693

Yes, I can do that.

chief1983

2015-05-07 17:57

administrator   ~0016698

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

niffiwan

2015-05-10 11:37

developer   ~0016701

fixed in aa6936a2a501ebea60366034afbe70a4c5f269ff

niffiwan

2015-05-10 11:39

developer   ~0016702

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

chief1983

2015-05-10 12:36

administrator   ~0016703

I gotcha man.

Issue History

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