View Issue Details

IDProjectCategoryView StatusLast Update
0002376FSSCPmodelspublic2012-11-28 07:32
ReporterFUBAR-BDHR Assigned ToThe_E  
PrioritynormalSeveritymajorReproducibilityalways
Status closedResolutionunable to reproduce 
Product Version3.6.13 
Summary0002376: Cockpit models not rendered in nebula or cause outright crash
DescriptionI've noticed in recent testing that cockpit models do not render at all in a nebula. If you play a mission with the same ship then load a nebula mission you get the following crash:

Assert: Polygon_models[num]->id == model_num
File: modelread.cpp
Line: 2832

<no module>! KiFastSystemCallRet
<no module>! WaitForSingleObject + 18 bytes
<no module>! SCP_DumpStack + 354 bytes
<no module>! WinAssert + 208 bytes
<no module>! model_get + 256 bytes
<no module>! ship_add_cockpit_display + 105 bytes
<no module>! ship_init_cockpit_displays + 283 bytes
<no module>! game_enter_state + 1171 bytes
<no module>! gameseq_set_state + 310 bytes
<no module>! game_process_event + 426 bytes
<no module>! gameseq_process_events + 152 bytes
<no module>! game_main + 782 bytes
<no module>! WinMain + 330 bytes
<no module>! __tmainCRTStartup + 358 bytes
<no module>! WinMainCRTStartup + 15 bytes
<no module>! RegisterWaitForInputIdle + 73 bytes
TagsNo tags attached.

Activities

Echelon9

2011-01-10 11:17

developer   ~0012591

If you comment out line modelread.cpp:2827 in the current trunk, and when playing with this modified build you hit instead Assert( num < MAX_POLYGON_MODELS ) -- then it's almost 100% going to be because we've hit the limit of Polygon_models[MAX_POLYGON_MODELS].

MAX_POLYGON_MODELS is 128 in standard builds, 300 in Inferno.

I think this will only show up in nebulas as the nebula lightning strikes also use Polygon_models array.

*sigh* fixed length arrays....

FUBAR-BDHR

2011-01-10 22:17

developer   ~0012592

Last edited: 2011-01-10 22:38

OK that makes no sense. Line 2827 is:

int num = model_num % MAX_POLYGON_MODELS;

If it comment that out num won't even be defined and the next 2 lines are asserts based on it including the one you listed. Also the assert it is hitting is the last one in that function so it's already passed the num checks.

Just to be on the safe side when it asserts num = 5.

Just to be sure I bumped MAX_POLYGON_MODELS to 500 for a quick test. no difference.

Echelon9

2011-01-11 13:06

developer   ~0012594

Sorry, what I'm trying to show is that line 2827 is a poor form of boundary case checking to ensure we don't overrun the fixed array Polygon_models[].

That num = 5 (low number) indicates to me model_num is probably MAX_POLYGON_MODELS + 5 at the point of the Assert().

Echelon9

2012-04-03 13:38

developer   ~0013447

Is there a testing mission to reproduce?

FUBAR-BDHR

2012-11-20 16:38

developer   ~0014127

This should be reproducible with any ships with a cockpit model in Diaspora. All you need is a mission with default player ship and set full nebula in FRED.

The_E

2012-11-20 20:14

administrator  

2376.patch (4,762 bytes)   
Index: model/modelread.cpp
===================================================================
--- model/modelread.cpp	(revision 9365)
+++ model/modelread.cpp	(working copy)
@@ -54,7 +54,7 @@
 
 // info for special polygon lists
 
-polymodel *Polygon_models[MAX_POLYGON_MODELS];
+SCP_vector<polymodel*> Polygon_models;
 SCP_vector<polymodel_instance*> Polygon_model_instances;
 
 SCP_vector<bsp_collision_tree> Bsp_collision_tree_list;
@@ -125,15 +125,12 @@
 // With the basic page in system this can be called from outside of modelread.cpp
 void model_unload(int modelnum, int force)
 {
-	int i, j, num;
+	int i, j;
+	size_t num;
 
-	if ( modelnum >= MAX_POLYGON_MODELS ) {
-		num = modelnum % MAX_POLYGON_MODELS;
-	} else {
-		num = modelnum;
-	}
+	num = modelnum;
 
-	if ( (num < 0) || (num >= MAX_POLYGON_MODELS))	{
+	if ( (num < 0) || (num >= Polygon_models.size()))	{
 		return;
 	}
 
@@ -286,8 +283,6 @@
 
 void model_free_all()
 {
-	int i;
-
 	if ( !model_initted)	{
 		model_init();
 		return;
@@ -296,7 +291,7 @@
 	mprintf(( "Freeing all existing models...\n" ));
 	model_instance_free_all();
 
-	for (i=0;i<MAX_POLYGON_MODELS;i++) {
+	for (size_t i = 0; i<Polygon_models.size(); i++) {
 		// forcefully unload all loaded models (be careful with this)
 		model_unload(i, 1);		
 	}
@@ -318,8 +313,6 @@
 
 void model_page_in_start()
 {
-	int i;
-
 	if ( !model_initted ) {
 		model_init();
 		return;
@@ -327,35 +320,31 @@
 
 	mprintf(( "Starting model page in...\n" ));
 
-	for (i=0; i<MAX_POLYGON_MODELS; i++) {
-		if (Polygon_models[i] != NULL)
-			Polygon_models[i]->used_this_mission = 0;
+	for (SCP_vector<polymodel*>::iterator pmit = Polygon_models.begin(); pmit != Polygon_models.end(); ++pmit) {
+		if ((*pmit) != NULL)
+			(*pmit)->used_this_mission = 0;
 	}
 }
 
 void model_page_in_stop()
 {
-	int i;
-
 	Assert( model_initted );
 
 	mprintf(( "Stopping model page in...\n" ));
 
-	for (i=0; i<MAX_POLYGON_MODELS; i++) {
-		if (Polygon_models[i] == NULL)
+	for (SCP_vector<polymodel*>::iterator pmit = Polygon_models.begin(); pmit != Polygon_models.end(); ++pmit) {
+		if ((*pmit) == NULL)
 			continue;
 
-		if (Polygon_models[i]->used_this_mission)
+		if ((*pmit)->used_this_mission)
 			continue;
-	
-		model_unload(i);
+
+		model_unload(pmit - Polygon_models.begin());
 	}
 }
 
 void model_init()
 {
-	int i;
-
 	if ( model_initted )		{
 		Int3();		// Model_init shouldn't be called twice!
 		return;
@@ -365,9 +354,7 @@
 	Model_ram = 0;
 #endif
 
-	for (i=0;i<MAX_POLYGON_MODELS;i++) {
-		Polygon_models[i] = NULL;
-	}
+	Polygon_models.clear();
 
 	atexit( model_free_all );
 	model_initted = 1;
@@ -2408,7 +2395,7 @@
 
 	num = -1;
 
-	for (i=0; i< MAX_POLYGON_MODELS; i++)	{
+	for (i=0; i< (int)Polygon_models.size(); i++)	{
 		if ( Polygon_models[i] )	{
 			if (!stricmp(filename, Polygon_models[i]->filename) && !duplicate)		{
 				// Model already loaded; just return.
@@ -2421,6 +2408,9 @@
 		}
 	}
 
+	if (Polygon_models.size() == 0)
+		num = 0;
+
 	// No empty slot
 	if ( num == -1 )	{
 		Error( LOCATION, "Too many models" );
@@ -2431,22 +2421,15 @@
 
 	pm = (polymodel *)vm_malloc( sizeof(polymodel) );
 	Assert( pm != NULL );
+
+	memset(pm, 0, sizeof(polymodel));
 	
-	Polygon_models[num] = pm;
-	
-	memset(pm, 0, sizeof(polymodel));
+	Polygon_models.push_back(pm);
 
 	pm->n_paths = 0;
 	pm->paths = NULL;
 
-	int org_sig = Model_signature;
-	Model_signature+=MAX_POLYGON_MODELS;
-	if ( Model_signature < org_sig )	{
-		Model_signature = 0;
-	}
-	Assert( (Model_signature % MAX_POLYGON_MODELS) == 0 );
-	pm->id = Model_signature + num;
-	Assert( (pm->id % MAX_POLYGON_MODELS) == num );
+	pm->id = Polygon_models.size()-1;
 
 	extern int Parse_normal_problem_count;
 	Parse_normal_problem_count = 0;
@@ -2940,17 +2923,10 @@
 		return NULL;
 	}
 
-	int num = model_num % MAX_POLYGON_MODELS;
-	
-	Assertion( num >= 0, "Model id %d is invalid. Please backtrace and investigate.\n", num);
-	Assertion( num < MAX_POLYGON_MODELS, "Model id %d is larger than MAX_POLYGON_MODELS (%d). This is impossible, thus we have to conclude that math as we know it has ceased to work.\n", num, MAX_POLYGON_MODELS );
-	Assertion( Polygon_models[num], "No model with id %d found. Please backtrace and investigate.\n", num );
-	Assertion( Polygon_models[num]->id == model_num, "Index collision between model %s and requested model %d. Please backtrace and investigate.\n", Polygon_models[num]->filename, model_num );
-
-	if (num < 0 || num > MAX_POLYGON_MODELS || !Polygon_models[num] || Polygon_models[num]->id != model_num)
+	if (model_num < (int)Polygon_models.size())
+		return Polygon_models[model_num];
+	else
 		return NULL;
-
-	return Polygon_models[num];
 }
 
 polymodel_instance* model_get_instance(int model_instance_num)
2376.patch (4,762 bytes)   

The_E

2012-11-20 20:15

administrator   ~0014130

I've attached a patch that makes Polygon_models dynamic. Please test.

Valathil

2012-11-20 20:28

developer   ~0014132

oops wrong bug

The_E

2012-11-28 07:32

administrator   ~0014204

Closing as unable to reproduce.

Issue History

Date Modified Username Field Change
2011-01-09 10:41 FUBAR-BDHR New Issue
2011-01-10 11:17 Echelon9 Note Added: 0012591
2011-01-10 22:17 FUBAR-BDHR Note Added: 0012592
2011-01-10 22:38 FUBAR-BDHR Note Edited: 0012592
2011-01-11 13:06 Echelon9 Note Added: 0012594
2012-04-03 13:38 Echelon9 Note Added: 0013447
2012-04-03 13:38 Echelon9 Status new => feedback
2012-11-20 16:38 FUBAR-BDHR Note Added: 0014127
2012-11-20 16:38 FUBAR-BDHR Status feedback => new
2012-11-20 20:14 The_E File Added: 2376.patch
2012-11-20 20:15 The_E Note Added: 0014130
2012-11-20 20:27 Valathil Assigned To => Valathil
2012-11-20 20:27 Valathil Status new => assigned
2012-11-20 20:28 Valathil Status assigned => resolved
2012-11-20 20:28 Valathil Resolution open => fixed
2012-11-20 20:28 Valathil Note Added: 0014132
2012-11-20 20:28 Valathil Status resolved => feedback
2012-11-20 20:28 Valathil Resolution fixed => reopened
2012-11-20 20:28 Valathil Status feedback => new
2012-11-20 20:30 Echelon9 Status new => code review
2012-11-20 20:31 Valathil Assigned To Valathil => The_E
2012-11-20 20:31 Valathil Status code review => assigned
2012-11-28 07:32 The_E Note Added: 0014204
2012-11-28 07:32 The_E Status assigned => closed
2012-11-28 07:32 The_E Resolution reopened => unable to reproduce