Merged script interpreter improvements from Dhewm3 #835

neo/d3xp/script/Script_Compiler.cpp

@@ -1255,6 +1255,26 @@ idVarDef* idCompiler::EmitFunctionParms( int op, idVarDef* func, int startarg, i
 		// need arg size separate since script object may be NULL
 		statement_t& statement = gameLocal.program.GetStatement( gameLocal.program.NumStatements() - 1 );
 		statement.c = SizeConstant( func->value.functionPtr->parmTotal );
+		// DG: before changes I did to ParseFunctionDef(), func->value.functionPtr->parmTotal was 0
+		//     if the function declaration/prototype has been parsed already, but the
+		//     definition/implementation hadn't been parsed yet. That was wrong and sometimes
+		//     (with debug game DLLs) lead to assertions in custom scripts, because the
+		//     stack space reserved for function parameters was wrong.
+		//     Now func->value.functionPtr->parmTotal is calculated when parsing the prototype,
+		//     but func->value.functionPtr->parmSize[i] is still only calculated when parsing
+		//     the implementation (as it's not needed before and so we can tell the cases apart here).
+		//     However, savegames from before the change have script checksums
+		//     (by idProgram::CalculateChecksum()) from statements with the wrong size, so
+		//     loading them would fail as the checksum doesn't match.
+		//     Setting this flag allows using the parmTotal argSize 0 when calculating the checksum
+		//     so it matches the one from old savegames (unless something else has also changed in
+		//     the script state so they really are incompatible). That's only done when actually
+		//     loading old savegames (detected via BUILD_NUMBER/savegame.GetBuildNumber())
+		if( op == OP_OBJECTCALL && func->value.functionPtr->parmTotal > 0
+				&& func->value.functionPtr->parmSize.Num() == 0 )
+		{
+			statement.flags = statement_t::FLAG_OBJECTCALL_IMPL_NOT_PARSED_YET;
+		}
 	}
 	else
 	{
@@ -2638,16 +2658,38 @@ void idCompiler::ParseFunctionDef( idTypeDef* returnType, const char* name )
 		}
 	}
 
-	// check if this is a prototype or declaration
+	// DG: make sure parmTotal gets calculated when parsing prototype (not just when parsing
+	//     implementation) so calling this function/method before the implementation has been parsed
+	//     works without getting Assertions in IdInterpreter::Execute() and ::LeaveFunction()
+	//     ("st->c->value.argSize == func->parmTotal", "localstackUsed == localstackBase", see #303 and #344)
+
+	// calculate stack space used by parms
+	numParms = type->NumParameters();
 	if( !CheckToken( "{" ) )
 	{
 		// it's just a prototype, so get the ; and move on
 		ExpectToken( ";" );
+		// DG: BUT only after calculating the stack space for the arguments because this
+		// function might be called before the implementation is parsed (see #303 and #344)
+		// which otherwise causes Assertions in IdInterpreter::Execute() and ::LeaveFunction()
+		// ("st->c->value.argSize == func->parmTotal", "localstackUsed == localstackBase")
+		func->parmTotal = 0;
+		for( i = 0; i < numParms; i++ )
+		{
+			parmType = type->GetParmType( i );
+			int size = parmType->Inherits( &type_object ) ? type_object.Size() : parmType->Size();
+			func->parmTotal += size;
+			// NOTE: Don't set func->parmSize[] yet, the workaround to keep compatibility
+			//       with old savegames checks for func->parmSize.Num() == 0
+			//       (see EmitFunctionParms() for more explanation of that workaround)
+			// Also not defining the parms yet, otherwise they're defined in a different order
+			// than before, so their .num is different which breaks compat with old savegames
+		}
 		return;
 	}
 
-	// calculate stack space used by parms
-	numParms = type->NumParameters();
+
+	int totalSize = 0; // DG: totalsize might already have been calculated for the prototype, see a few lines above
 	func->parmSize.SetNum( numParms );
 	for( i = 0; i < numParms; i++ )
 	{
@@ -2660,8 +2702,11 @@ void idCompiler::ParseFunctionDef( idTypeDef* returnType, const char* name )
 		{
 			func->parmSize[ i ] = parmType->Size();
 		}
-		func->parmTotal += func->parmSize[ i ];
+		totalSize += func->parmSize[ i ];
 	}
+	// DG: if parmTotal has been calculated before, it shouldn't have changed
+	assert( ( func->parmTotal == 0 || totalSize == func->parmTotal ) && "function parameter sizes differ between protype vs implementation?!" );
+	func->parmTotal = totalSize;
 
 	// define the parms
 	for( i = 0; i < numParms; i++ )
@@ -3175,6 +3220,8 @@ void idCompiler::CompileFile( const char* text, const char* filename, bool toCon
 
 	compile_time.Start();
 
+	idStr origFileName = filename; // DG: filename pointer might become invalid when calling NextToken() below
+
 	scope				= &def_namespace;
 	basetype			= NULL;
 	callthread			= false;
@@ -3258,6 +3305,11 @@ void idCompiler::CompileFile( const char* text, const char* filename, bool toCon
 	compile_time.Stop();
 	if( !toConsole )
 	{
-		gameLocal.Printf( "Compiled '%s': %.1f ms\n", filename, compile_time.Milliseconds() );
+		// DG: filename can be overwritten by NextToken() (via gameLocal.program.GetFilenum()), so
+		//     use a copy, origFileName, that's still valid here. Furthermore, the path is nonsense,
+		//     as idProgram::CompileText() called fileSystem->RelativePathToOSPath() on it
+		//     which does not return the *actual* full path of that file but invents one,
+		//     so revert that to the relative filename which at least isn't misleading
+		gameLocal.Printf( "Compiled '%s': %u ms\n", fileSystem->OSPathToRelativePath( origFileName ), compile_time.Milliseconds() );
 	}
 }

neo/d3xp/script/Script_Interpreter.cpp

@@ -214,9 +214,9 @@ bool idInterpreter::GetRegisterValue( const char* name, idStr& out, int scopeDep
 	idVarDef*		d;
 	char			funcObject[ 1024 ];
 	char*			funcName;
-	const idVarDef*	scope;
+	const idVarDef*	scope = NULL;
+	const idVarDef*	scopeObj;
 	const idTypeDef*	field;
-	const idScriptObject* obj;
 	const function_t* func;
 
 	out.Empty();
@@ -244,42 +244,46 @@ bool idInterpreter::GetRegisterValue( const char* name, idStr& out, int scopeDep
 	if( funcName )
 	{
 		*funcName = '\0';
-		scope = gameLocal.program.GetDef( NULL, funcObject, &def_namespace );
+		scopeObj = gameLocal.program.GetDef( NULL, funcObject, &def_namespace );
 		funcName += 2;
+		if( scopeObj )
+		{
+			scope = gameLocal.program.GetDef( NULL, funcName, scopeObj );
+		}
 	}
 	else
 	{
 		funcName = funcObject;
-		scope = &def_namespace;
+		scope = gameLocal.program.GetDef( NULL, func->Name(), &def_namespace );
+		scopeObj = NULL;
 	}
 
-	// Get the function from the object
-	d = gameLocal.program.GetDef( NULL, funcName, scope );
-	if( !d )
+	if( !scope )
 	{
 		return false;
 	}
 
-	// Get the variable itself and check various namespaces
-	d = gameLocal.program.GetDef( NULL, name, d );
+	d = gameLocal.program.GetDef( NULL, name, scope );
+
+	// Check the objects for it if it wasnt local to the function
 	if( !d )
 	{
-		if( scope == &def_namespace )
+		for( ; scopeObj && scopeObj->TypeDef()->SuperClass(); scopeObj = scopeObj->TypeDef()->SuperClass()->def )
 		{
-			return false;
-		}
-
-		d = gameLocal.program.GetDef( NULL, name, scope );
-		if( !d )
-		{
-			d = gameLocal.program.GetDef( NULL, name, &def_namespace );
-			if( !d )
+			d = gameLocal.program.GetDef( NULL, name, scopeObj );
+			if( d )
 			{
-				return false;
+				break;
 			}
 		}
 	}
 
+	if( !d )
+	{
+		out = "???";
+		return false;
+	}
+
 	reg = GetVariable( d );
 	switch( d->Type() )
 	{
@@ -320,15 +324,25 @@ bool idInterpreter::GetRegisterValue( const char* name, idStr& out, int scopeDep
 			break;
 
 		case ev_field:
+		{
+			idEntity*		entity;
+			idScriptObject*	obj;
+
 			if( scope == &def_namespace )
 			{
 				// should never happen, but handle it safely anyway
 				return false;
 			}
 
-			field = scope->TypeDef()->GetParmType( reg.ptrOffset )->FieldType();
-			obj   = *reinterpret_cast<const idScriptObject**>( &localstack[ callStack[ callStackDepth ].stackbase ] );
-			if( !field || !obj )
+			field  = d->TypeDef()->FieldType();
+			entity = GetEntity( *( ( int* )&localstack[ localstackBase ] ) );
+			if( !entity || !field )
+			{
+				return false;
+			}
+
+			obj = &entity->scriptObject;
+			if( !obj )
 			{
 				return false;
 			}
@@ -343,10 +357,29 @@ bool idInterpreter::GetRegisterValue( const char* name, idStr& out, int scopeDep
 					out = va( "%g", *( reinterpret_cast<float*>( &obj->data[ reg.ptrOffset ] ) ) );
 					return true;
 
+				case ev_string:
+				{
+					const char* str;
+					str = reinterpret_cast<const char*>( &obj->data[ reg.ptrOffset ] );
+					if( !str )
+					{
+						out = "\"\"";
+					}
+					else
+					{
+						out  = "\"";
+						out += str;
+						out += "\"";
+					}
+					return true;
+				}
+
 				default:
 					return false;
 			}
+
 			break;
+		}
 
 		case ev_string:
 			if( reg.stringPtr )

neo/d3xp/script/Script_Program.cpp

@@ -1914,7 +1914,9 @@ statement_t* idProgram::AllocStatement()
 		gameLocal.Error( "Exceeded maximum allowed number of statements (%d)", statements.Max() );
 #endif
 	}
-	return statements.Alloc();
+	statement_t* ret = statements.Alloc();
+	ret->flags = 0; // DG: initialize the added flags (that are rarely set/used otherwise) to 0
+	return ret;
 }
 
 /*
@@ -2343,7 +2345,7 @@ void idProgram::Save( idSaveGame* savefile ) const
 		savefile->WriteByte( variables[i] );
 	}
 
-	int checksum = CalculateChecksum();
+	int checksum = CalculateChecksum( false );
 	savefile->WriteInt( checksum );
 }
 
@@ -2381,10 +2383,12 @@ bool idProgram::Restore( idRestoreGame* savefile )
 	int saved_checksum, checksum;
 
 	savefile->ReadInt( saved_checksum );
-	checksum = CalculateChecksum();
+	bool isOldSavegame = savefile->GetBuildNumber() <= BUILD_NUMBER_SAVE_VERSION_CHANGE;
+	checksum = CalculateChecksum( isOldSavegame );
 
 	if( saved_checksum != checksum )
 	{
+		gameLocal.Warning( "WARNING: Real Script checksum didn't match the one from the savegame!" );
 		result = false;
 	}
 
@@ -2396,7 +2400,7 @@ bool idProgram::Restore( idRestoreGame* savefile )
 idProgram::CalculateChecksum
 ================
 */
-int idProgram::CalculateChecksum() const
+int idProgram::CalculateChecksum( bool forOldSavegame ) const
 {
 	int i, result;
 
@@ -2414,6 +2418,20 @@ int idProgram::CalculateChecksum() const
 
 	memset( statementList, 0, ( sizeof( statementBlock_t ) * statements.Num() ) );
 
+	// DG hack: get the vardef for the argSize == 0 constant for savegame-compat
+	int constantZeroNum = -1;
+	if( forOldSavegame )
+	{
+		for( idVarDef* def = GetDefList( "<IMMEDIATE>" ); def != NULL; def = def->Next() )
+		{
+			if( def->Type() == ev_argsize && def->value.argSize == 0 )
+			{
+				constantZeroNum = def->num;
+				break;
+			}
+		}
+	}
+
 	// Copy info into new list, using the variable numbers instead of a pointer to the variable
 	for( i = 0; i < statements.Num(); i++ )
 	{
@@ -2437,7 +2455,18 @@ int idProgram::CalculateChecksum() const
 		}
 		if( statements[i].c )
 		{
-			statementList[i].c = statements[i].c->num;
+			// DG: old savegames wrongly assumed argSize 0 for some statements.
+			//     So for the checksums to match we need to use the corresponding vardef num here
+			//     See idCompiler::EmitFunctionParms() and ParseFunctionDef() for more details.
+			if( forOldSavegame && statements[i].op == OP_OBJECTCALL
+					&& statements[i].flags == statement_t::FLAG_OBJECTCALL_IMPL_NOT_PARSED_YET )
+			{
+				statementList[i].c = constantZeroNum;
+			}
+			else
+			{
+				statementList[i].c = statements[i].c->num;
+			}
 		}
 		else
 		{

neo/d3xp/script/Script_Program.h

@@ -42,9 +42,7 @@ class idRestoreGame;
 #define MAX_STRING_LEN		128
 #define MAX_GLOBALS			296608			// in bytes
 #define MAX_STRINGS			1024
-
 #define MAX_FUNCS			3584
-
 #define MAX_STATEMENTS		131072			// statement_t - 18 bytes last I checked
 
 typedef enum
@@ -317,9 +315,9 @@ typedef union varEval_s
 	float*					floatPtr;
 	idVec3*					vectorPtr;
 	function_t*				functionPtr;
-	int*					 intPtr;
+	int*					intPtr;
 	byte*					bytePtr;
-	int*					 entityNumberPtr;
+	int*					entityNumberPtr;
 	int						virtualFunction;
 	int						jumpOffset;
 	int						stackOffset;		// offset in stack for local variables
@@ -335,9 +333,9 @@ class idVarDef
 	friend class idVarDefName;
 
 public:
-	int						num;
+	int						num;			// global index/ID of variable
 	varEval_t				value;
-	idVarDef* 				scope; 			// function, namespace, or object the var was defined in
+	idVarDef* 				scope;			// function, namespace, or object the var was defined in
 	int						numUsers;		// number of users if this is a constant
 
 	typedef enum
@@ -464,11 +462,20 @@ extern	idVarDef	def_boolean;
 typedef struct statement_s
 {
 	unsigned short	op;
+	unsigned short	flags; // DG: added this for ugly hacks
+	enum
+	{
+		// op is OP_OBJECTCALL and when the statement was created the function/method
+		// implementation hasn't been parsed yet (only the declaration/prototype)
+		// see idCompiler::EmitFunctionParms() and idProgram::CalculateChecksum()
+		FLAG_OBJECTCALL_IMPL_NOT_PARSED_YET = 1,
+	};
+	// DG: moved linenumber and file up here to prevent wasting 8 bytes of padding on 64bit
+	unsigned short	linenumber;
+	unsigned short	file;
 	idVarDef*		a;
 	idVarDef*		b;
 	idVarDef*		c;
-	unsigned short	linenumber;
-	unsigned short	file;
 } statement_t;
 
 /***********************************************************************
@@ -509,6 +516,8 @@ private:
 	int											top_files;
 
 	void										CompileStats();
+	byte*										ReserveDefMemory( int size );
+	idVarDef*									AllocVarDef( idTypeDef* type, const char* name, idVarDef* scope );
 
 public:
 	idVarDef*									returnDef;
@@ -520,7 +529,7 @@ public:
 	// save games
 	void										Save( idSaveGame* savefile ) const;
 	bool										Restore( idRestoreGame* savefile );
-	int											CalculateChecksum() const;		// Used to insure program code has not
+	int											CalculateChecksum( bool forOldSavegame ) const;		// Used to insure program code has not
 	//    changed between savegames
 
 	void										Startup( const char* defaultScript );
@@ -544,13 +553,6 @@ public:
 	idTypeDef*									GetType( idTypeDef& type, bool allocate );
 	idTypeDef*									FindType( const char* name );
 
-	// RB begin
-private:
-	byte*										ReserveDefMemory( int size );
-	idVarDef*									AllocVarDef( idTypeDef* type, const char* name, idVarDef* scope );
-public:
-	// RB end
-
 	idVarDef*									AllocDef( idTypeDef* type, const char* name, idVarDef* scope, bool constant );
 	idVarDef*									GetDef( const idTypeDef* type, const char* name, const idVarDef* scope ) const;
 	void										FreeDef( idVarDef* d, const idVarDef* scope );

neo/framework/BuildVersion.h

@@ -26,8 +26,9 @@ If you have questions concerning this license or the applicable additional terms
 ===========================================================================
 */
 
-const int BUILD_NUMBER_SAVE_VERSION_BEFORE_SKIP_CINEMATIC = 1400;
-const int BUILD_NUMBER_SAVE_VERSION_CHANGE			= 1401;		// Altering saves so that the version goes in the Details file that we read in during the enumeration phase
+const int BUILD_NUMBER_SAVE_VERSION_BEFORE_SKIP_CINEMATIC	= 1400;
+const int BUILD_NUMBER_SAVE_VERSION_CHANGE					= 1401;		// Altering saves so that the version goes in the Details file that we read in during the enumeration phase
+const int BUILD_NUMBER_SAVE_VERSION_SCRIPT_CHANGES1			= 1402;		// RB: Merged script compiler changes from Dhewm3 so functions don't need declarations before used
 
-const int BUILD_NUMBER = BUILD_NUMBER_SAVE_VERSION_CHANGE;
+const int BUILD_NUMBER = BUILD_NUMBER_SAVE_VERSION_SCRIPT_CHANGES1;
 const int BUILD_NUMBER_MINOR = 0;

neo/tools/imgui/lighteditor/LightEditor.cpp

@@ -1224,7 +1224,7 @@ void LightEditor::Draw()
 		TempApplyChanges();
 	}
 
-exitLightEditor:
+//exitLightEditor:
 
 	if( isShown && !showTool )
 	{