From 96ebce54b144dcea22af6177251859e1a6550945 Mon Sep 17 00:00:00 2001 From: Robert Beckebans Date: Sat, 16 Dec 2023 15:48:35 +0100 Subject: [PATCH] Merged script interpreter improvements from Dhewm3 #835 --- neo/d3xp/script/Script_Compiler.cpp | 62 +++++++++++++++-- neo/d3xp/script/Script_Interpreter.cpp | 77 +++++++++++++++------ neo/d3xp/script/Script_Program.cpp | 39 +++++++++-- neo/d3xp/script/Script_Program.h | 34 ++++----- neo/framework/BuildVersion.h | 7 +- neo/tools/imgui/lighteditor/LightEditor.cpp | 2 +- 6 files changed, 169 insertions(+), 52 deletions(-) diff --git a/neo/d3xp/script/Script_Compiler.cpp b/neo/d3xp/script/Script_Compiler.cpp index d6aa17c1..dd52e434 100644 --- a/neo/d3xp/script/Script_Compiler.cpp +++ b/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() ); } } diff --git a/neo/d3xp/script/Script_Interpreter.cpp b/neo/d3xp/script/Script_Interpreter.cpp index 8c8d63da..6e2d64f4 100644 --- a/neo/d3xp/script/Script_Interpreter.cpp +++ b/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( &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( &obj->data[ reg.ptrOffset ] ) ) ); return true; + case ev_string: + { + const char* str; + str = reinterpret_cast( &obj->data[ reg.ptrOffset ] ); + if( !str ) + { + out = "\"\""; + } + else + { + out = "\""; + out += str; + out += "\""; + } + return true; + } + default: return false; } + break; + } case ev_string: if( reg.stringPtr ) diff --git a/neo/d3xp/script/Script_Program.cpp b/neo/d3xp/script/Script_Program.cpp index 82aab5e9..214c1632 100644 --- a/neo/d3xp/script/Script_Program.cpp +++ b/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( "" ); 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 { diff --git a/neo/d3xp/script/Script_Program.h b/neo/d3xp/script/Script_Program.h index 4caa4b6c..d8eba231 100644 --- a/neo/d3xp/script/Script_Program.h +++ b/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 ); diff --git a/neo/framework/BuildVersion.h b/neo/framework/BuildVersion.h index 95a0cbd3..de76a91a 100644 --- a/neo/framework/BuildVersion.h +++ b/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; diff --git a/neo/tools/imgui/lighteditor/LightEditor.cpp b/neo/tools/imgui/lighteditor/LightEditor.cpp index 9dbf59a0..6c4521b0 100644 --- a/neo/tools/imgui/lighteditor/LightEditor.cpp +++ b/neo/tools/imgui/lighteditor/LightEditor.cpp @@ -1224,7 +1224,7 @@ void LightEditor::Draw() TempApplyChanges(); } -exitLightEditor: +//exitLightEditor: if( isShown && !showTool ) {