View Issue Details

IDProjectCategoryView StatusLast Update
0000925FSSCPgameplaypublic2006-06-03 21:21
Reporterkarajorma Assigned ToGoober5000  
PrioritynormalSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
Summary0000925: Special arguments can't be used in sexps that preload their arguments
DescriptionBasically if I make a mission like this

$Formula: ( when-argument
   ( random-of
      "GTF Ulysses"
      "GTF Hercules Mark II"
      "GTF Ares"
   )
   ( true )
   ( change-ship-class
      "<argument>"
      "Beta 1"
   )
)

FS2 debug builds crash out with this error

Assert: Sexp_current_replacement_argument != NULL
File: D:\C++\Freespace\fs2_open\code\Parse\SEXP.CPP
Line: 18140
[This filename points to the location of a file on the computer that built this executable]

Call stack:
------------------------------------------------------------------
    get_sexp() get_sexp() get_sexp_main() parse_event() parse_events() parse_mission() parse_main() mission_load() game_start_mission() game_enter_state() gameseq_set_state() game_process_event() gameseq_process_events() game_main() WinMain()------------------------------------------------------------------
Additional InformationI never did get too deeply into how the Argument code works when I got started so if Goober wants this one it's his.
TagsNo tags attached.

Activities

2006-05-25 19:19

 

karajorma

2006-05-25 19:19

administrator   ~0005622

Hmmm. Running this on the release build also gives an error

Error: Can't open model file <>
File:D:\C++\Freespace\fs2_open\code\Model\ModelRead.cpp
Line: 1810
[This filename points to the location of a file on the computer that built this executable]

Call stack:
------------------------------------------------------------------
------------------------------------------------------------------

Goober5000

2006-05-26 00:02

administrator   ~0005625

Nuts.

Goober5000

2006-06-03 05:33

administrator   ~0005721

Last edited: 2006-06-03 05:34

Oh, @#%$@!!

There's actually nothing wrong with the sexp at all... this happens during the preload routine, when change-ship-class looks for any models that will be used during the mission. The special argument isn't valid here, because the sexp isn't being executed.

There's really no good way to fix this. What would have to be done is to claw a path backwards up the sexp chain and run a preload callback for every special argument. The best solution would be to prevent any sexp that requires a preload from using a special argument.

The next best solution would be to just not preload any special arguments. I'm not sure what that would do for sexps expecting a preload, though. Taylor would have to chime in for that, as he wrote most of the preload code.

Adding a routine that traverses the sexp tree backwards looking for every possible special argument, plus a dozen or so preload callbacks, is a recipe for agonizing pain.

edited on: 06-03-06 01:34

taylor

2006-06-03 06:01

administrator   ~0005729

Hmm, we probably just need to be extra careful to guard against non-preload failure then. We don't actually prevent special arguments for things that preload, just make thing that things that normally preload, don't absolutely have to in order to work. This can be a little slower in mission, but that can be handled in other ways. I agree that trying to preload all of this would be a living hell to code and keep working so lets just ignore that as a possible option for now and figure something else out.

I'll have a look at this tonight and see what I can come up with.

Goober5000

2006-06-03 18:56

administrator   ~0005741

Actually, I found some code in invalidate-argument that might do what I need as far as finding the list of arguments. That would just leave a list of callbacks, which would be tedious and painful but not really hard.

I just need to know if it would be harmful to preload stuff that won't actually be in the mission, such as a set of skyboxes.

taylor

2006-06-03 19:36

administrator   ~0005742

If it's a choice, I'd rather not preload. Preloading isn't a requirement, it just makes certain situations during the mission slower. Preloading things that we don't need can waste a tremendous amount of memory so as far as bad things though, it ain't good. Unused ships and skyboxes could account for dozens of megs of memory being needed that wouldn't be otherwise.

It's cheap, but how about something like the attached diff. It works, it's simple, but doesn't really hurt anything. There are situations where the speed wouldn't be an issue, and situations where it would. But I'd much rather deal with a one-time speed drop while a POF/texture is getting loaded rather than the possibility of wasting 30+ meg on crap I don't need for the *entire* mission.

2006-06-03 19:36

 

sexp-cpp.patch (3,147 bytes)   
Index: code/parse/sexp.cpp
===================================================================
RCS file: /home/fs2source/cvsroot/fs2_open/code/parse/sexp.cpp,v
retrieving revision 2.260
diff -u -r2.260 sexp.cpp
--- code/parse/sexp.cpp	2 Jun 2006 09:29:13 -0000	2.260
+++ code/parse/sexp.cpp	3 Jun 2006 20:17:46 -0000
@@ -3694,7 +3694,7 @@
 	// Goober5000 - preload stuff for certain sexps
 	if (!Fred_running)
 	{
-		int n;
+		int n, tmp_index = -1;
 		ship_info *sip;
 
 		op = get_operator_const(CTEXT(start));
@@ -3705,9 +3705,22 @@
 				// model is argument #1
 				n = CDR(start);
 
+				// if we are a special argument then don't preload, the model will
+				// get loaded properly later, it may just be a little slower in game
+				if ( !strcmp(Sexp_nodes[n].text, SEXP_ARGUMENT_STRING) )
+					break;
+
+				tmp_index = ship_info_lookup( CTEXT(n) );
+
+				// just to be on the safe side
+				if ( tmp_index < 0 ) {
+					Int3();
+					break;
+				}
+
 				// preload the model, just in case there is no other ship of this class in the mission
 				// (this eliminates the slight pause during a mission when changing to a previously unloaded model)
-				sip = &Ship_info[ship_info_lookup(CTEXT(n))];										// we already know this class exists
+				sip = &Ship_info[tmp_index];		// we already know this class exists, or at least we hope it does
 				sip->modelnum = model_load(sip->pof_file, sip->n_subsystems, &sip->subsystems[0]);	// use the highest detail level
 				break;
 
@@ -3733,6 +3746,11 @@
 			case OP_SET_SKYBOX_MODEL:
 				n = CDR(start);
 
+				// if we are a special argument then don't preload, the model will
+				// get loaded properly later, it will just be a bit slower in game
+				if ( !strcmp(Sexp_nodes[n].text, SEXP_ARGUMENT_STRING) )
+					break;
+
 				sexp_set_skybox_model_preload( CTEXT(n) );
 				break;
 
@@ -3740,11 +3758,20 @@
 				// weapon to change to is arg #3
 				n = CDDDR(start);
 
+				// if we are a special argument then don't preload, the weapon will
+				// get loaded properly later, it may just be a little slower in game
+				if ( !strcmp(Sexp_nodes[n].text, SEXP_ARGUMENT_STRING) )
+					break;
+
 				weapon_mark_as_used( weapon_info_lookup(CTEXT(n) ));
 				break;
 
 			case OP_ADD_SUN_BITMAP:
 				n = CDR(start);
+				// if we are a special argument then don't preload, the bitmap will
+				// get loaded properly later, it may just be a little slower in game
+				if ( !strcmp(Sexp_nodes[n].text, SEXP_ARGUMENT_STRING) )
+					break;
 
 				stars_preload_sun_bitmap( CTEXT(n) );
 				break;
@@ -3752,6 +3779,11 @@
 			case OP_ADD_BACKGROUND_BITMAP:
 				n = CDR(start);
 
+				// if we are a special argument then don't preload, the bitmap will
+				// get loaded properly later, it may just be a little slower in game
+				if ( !strcmp(Sexp_nodes[n].text, SEXP_ARGUMENT_STRING) )
+					break;
+
 				stars_preload_background_bitmap( CTEXT(n) );
 				break;
 		}
@@ -10558,7 +10590,6 @@
 	// get the subsystems
 	for (; n >= 0; n = CDR(n))
 	{
-		char *subsystem_name = CTEXT(n);
 		ship_subsys *ss = ship_get_subsys(&Ships[ship_num], CTEXT(n));
 		if (ss == NULL)
 			continue;
sexp-cpp.patch (3,147 bytes)   

Goober5000

2006-06-03 20:05

administrator   ~0005744

Gah. I just finished adding the special argument preloader. :p

Your diff looks good though. You think it's better to err on the side of not preloading?

taylor

2006-06-03 20:57

administrator   ~0005747

:) Sorry, I should have posted earlier. I made those changes last night but then started working on the launcher and waited post here.

In general I'd say that making sure to preload it all would be a good thing. Unfortunately my mind can find the bad in any situation, and I see this coming back to bite us big time. I think it would be safer to not preload everything and see how that works out. Weapons and ships may already be loaded in a mission so they would, in effect, already be preloaded. Something like a skybox/skysphere can have some really large textures though and there is deffinitely somewhere that we'd get into trouble.

To be on the safe side, I say that we just ignore the special argument preloading. Memory is just to precious for us right now to risk wasting large amounts on this. I think I'd rather save the memory and do more work to speed up loading in those things while in-game (as opposed to preload).

Be sure to keep the code you have though since we could end up using it at some point. Someone will undoubtedly complain in the future about some slow loading.

Goober5000

2006-06-03 21:21

administrator   ~0005748

Okay. I think I'll commit a combination of my code and yours: keep the special arg preloading code, but disable it.

Marking this resolved.

Issue History

Date Modified Username Field Change
2006-05-25 19:08 karajorma New Issue
2006-05-25 19:19 karajorma File Added: Bug Test 06 - Random Of.fs2
2006-05-25 19:19 karajorma Note Added: 0005622
2006-05-26 00:02 Goober5000 Note Added: 0005625
2006-05-26 00:02 Goober5000 Status new => assigned
2006-05-26 00:02 Goober5000 Assigned To => Goober5000
2006-06-03 05:33 Goober5000 Note Added: 0005721
2006-06-03 05:34 Goober5000 Note Edited: 0005721
2006-06-03 05:34 Goober5000 Note Edited: 0005721
2006-06-03 05:35 Goober5000 Summary Assertions with certain uses of Random-of => Special arguments can't be used in sexps that preload their arguments
2006-06-03 06:01 taylor Note Added: 0005729
2006-06-03 18:56 Goober5000 Note Added: 0005741
2006-06-03 19:36 taylor Note Added: 0005742
2006-06-03 19:36 taylor File Added: sexp-cpp.patch
2006-06-03 20:05 Goober5000 Note Added: 0005744
2006-06-03 20:57 taylor Note Added: 0005747
2006-06-03 21:21 Goober5000 Status assigned => resolved
2006-06-03 21:21 Goober5000 Resolution open => fixed
2006-06-03 21:21 Goober5000 Note Added: 0005748