diff --git a/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/classes.py b/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/classes.py index a335ae384..6b5b60cde 100644 --- a/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/classes.py +++ b/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/classes.py @@ -1303,18 +1303,14 @@ def compile( this_stack = 0 # DEBUG: Log arguments before compilation - # Compile arguments in FORWARD order (left to right, first argument first) - # NCS bytecode pushes arguments left-to-right, so when the interpreter - # pops them (last-in-first-out), args_snap has them in reverse order. - # The interpreter then reverses args_snap to match function.params order. - # - Compile in forward: push fFloat (first param, bottom), push nWidth, push nDecimals (last param, top) - # - Stack: [fFloat, nWidth, nDecimals] (nDecimals on top) - # - Interpreter pops: nDecimals -> args_snap[0], nWidth -> args_snap[1], fFloat -> args_snap[2] - # - Before reverse: args_snap = [nDecimals, nWidth, fFloat] - # - After reverse: args_snap = [fFloat, nWidth, nDecimals] - CORRECT! - for i in range(len(self._args)): # Iterate in forward order - arg = self._args[i] - param_index = i # Parameter index in forward order + # Push the arguments LAST-first: the last parameter goes onto the stack first + # and the first parameter ends up on top, which is the order the NWScript VM + # (and the original compiler) expects for every engine action. Omitted + # trailing arguments have been filled with their defaults above, so they are + # pushed before the explicit ones. + # Example: SetGlobalNumber("A", 7) -> CONSTI 7, CONSTS "A", ACTION. + for param_index in range(len(self._args) - 1, -1, -1): + arg = self._args[param_index] param_type = DynamicDataType(self._function.params[param_index].datatype) if param_type == DataType.ACTION: after_command = NCSInstruction() diff --git a/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/interpreter.py b/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/interpreter.py index 5cc5b1b9c..c167050ae 100644 --- a/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/interpreter.py +++ b/Libraries/PyKotor/src/pykotor/resource/formats/ncs/compiler/interpreter.py @@ -427,9 +427,9 @@ def do_action(self, function: ScriptFunction, args: int): # noqa: C901, PLR0912 args_snap = [] - # Pop arguments from stack in reverse order (last param popped first) - for i in range(args): - param_index = args - 1 - i # Reverse order + # The compiler pushes the last parameter first, so the first parameter is on top + # of the stack: pop in parameter order. + for param_index in range(args): if param_index >= len(function.params): msg = f"Action '{function.name}' parameter index {param_index} out of range" raise ValueError(msg) @@ -456,20 +456,8 @@ def do_action(self, function: ScriptFunction, args: int): # noqa: C901, PLR0912 msg = f"Stack underflow while popping argument for '{function.name}'" raise RuntimeError(msg) from e - # DEBUG: Log args_snap before reverse - print( - f"DEBUG: args_snap before reverse: {[f'{obj.data_type.name}={obj.value}' for obj in args_snap]}" - ) - - # We compiled arguments in reverse order (last first), so when we pop them (last-in-first-out), - # args_snap[0] is the last parameter and args_snap[-1] is the first parameter. - # We need to reverse to match function.params order (first parameter at index 0). - args_snap.reverse() - - # DEBUG: Log args_snap after reverse and function params - print( - f"DEBUG: args_snap after reverse: {[f'{obj.data_type.name}={obj.value}' for obj in args_snap]}" - ) + # DEBUG: Log args_snap (already in function.params order) and function params + print(f"DEBUG: args_snap: {[f'{obj.data_type.name}={obj.value}' for obj in args_snap]}") print(f"DEBUG: function.params: {[(p.name, p.datatype.name) for p in function.params]}") # Validate argument types diff --git a/Libraries/PyKotor/tests/resource/formats/test_ncs.py b/Libraries/PyKotor/tests/resource/formats/test_ncs.py index e8c4ed368..373bdab32 100644 --- a/Libraries/PyKotor/tests/resource/formats/test_ncs.py +++ b/Libraries/PyKotor/tests/resource/formats/test_ncs.py @@ -4,6 +4,7 @@ import importlib.util import os import pathlib +import struct import sys import tempfile import textwrap @@ -77,6 +78,40 @@ def _ncs_dencs_available() -> bool: _TSL_NCS_TEST_FILES_DIR = Path(__file__).resolve().parents[3] / "test_files" +def _hand_assembled_ncs(*instructions: bytes) -> bytes: + """Assemble an NCS file from raw instruction bytes, independent of PyKotor's writer. + + Layout: "NCS V1.0", the 0x42 size marker, the total file size as a big-endian uint32, + then the instructions. Every program starts with ``JSR main; RETN`` (``main`` follows at +8). + """ + body = b"".join(instructions) + return b"NCS V1.0" + bytes([0x42]) + (13 + len(body)).to_bytes(4, "big") + body + + +_JSR_MAIN = bytes([0x1E, 0x00]) + (8).to_bytes(4, "big", signed=True) +_RETN = bytes([0x20, 0x00]) + + +def _consti(value: int) -> bytes: + return bytes([0x04, 0x03]) + value.to_bytes(4, "big", signed=True) + + +def _constf(value: float) -> bytes: + return bytes([0x04, 0x04]) + struct.pack(">f", value) + + +def _consts(value: str) -> bytes: + return bytes([0x04, 0x05]) + len(value).to_bytes(2, "big") + value.encode("ascii") + + +def _consto(value: int) -> bytes: + return bytes([0x04, 0x06]) + value.to_bytes(4, "big", signed=True) + + +def _action(routine_id: int, argc: int) -> bytes: + return bytes([0x05, 0x00]) + routine_id.to_bytes(2, "big") + bytes([argc]) + + # ============================================================================ # Test Base Classes # ============================================================================ @@ -299,6 +334,91 @@ def test_enginecall_GetFactionEqual(self): """ ) + def test_enginecall_argument_order_bytes_set_global_number(self): + """Engine call arguments are pushed last-first, so the first argument is on top of the stack. + + NSS ``SetGlobalNumber("A", 7)`` must assemble to CONSTI 7, CONSTS "A", ACTION 581 (2 args). + The expected bytes are hand-assembled; the Interpreter is deliberately not involved, because it + used to carry a compensating bug that hid a swapped argument order. + """ + actual = bytes_ncs(compile_nss('void main() { SetGlobalNumber("A", 7); }', Game.K1)) + expected = _hand_assembled_ncs( + _JSR_MAIN, + _RETN, + _consti(7), # nValue, the last parameter, is pushed first + _consts("A"), # sIdentifier, the first parameter, ends up on top + _action(581, 2), # SetGlobalNumber + _RETN, + ) + assert actual == expected, f"{actual.hex(' ')} != {expected.hex(' ')}" + + def test_enginecall_argument_order_bytes_three_mixed_types(self): + """A three-argument call with object, int and float parameters pushes float, int, object.""" + actual = bytes_ncs( + compile_nss("void main() { SWMG_SetGunBankSpeed(OBJECT_SELF, 2, 1.5); }", Game.K1) + ) + expected = _hand_assembled_ncs( + _JSR_MAIN, + _RETN, + _constf(1.5), # fSpeed + _consti(2), # nGunBank + _consto(0), # oVehicle = OBJECT_SELF (on top) + _action(637, 3), # SWMG_SetGunBankSpeed + _RETN, + ) + assert actual == expected, f"{actual.hex(' ')} != {expected.hex(' ')}" + + def test_enginecall_argument_order_bytes_omitted_defaults(self): + """Omitted trailing arguments take their defaults and are pushed before the explicit ones. + + ``ActionMoveToObject(object oMoveTo, int bRun=FALSE, float fRange=1.0)`` called with only the + object pushes fRange (1.0), bRun (0), then oMoveTo. + """ + actual = bytes_ncs(compile_nss("void main() { ActionMoveToObject(OBJECT_SELF); }", Game.K1)) + expected = _hand_assembled_ncs( + _JSR_MAIN, + _RETN, + _constf(1.0), # default fRange + _consti(0), # default bRun + _consto(0), # oMoveTo = OBJECT_SELF (on top) + _action(22, 3), # ActionMoveToObject + _RETN, + ) + assert actual == expected, f"{actual.hex(' ')} != {expected.hex(' ')}" + + def test_enginecall_argument_order_bytes_action_argument(self): + """An action argument is pushed (as STORE_STATE + JMP + body + RETN) before the preceding arguments. + + ``DelayCommand(float fSeconds, action aActionToDelay)`` therefore emits the stored action first, + then the float, then ACTION 7. The body of the action is itself a last-first engine call. + """ + actual = bytes_ncs( + compile_nss('void main() { DelayCommand(1.0, SetGlobalNumber("A", 7)); }', Game.K1) + ) + action_body = b"".join( + [ + _consti(7), + _consts("A"), + _action(581, 2), # SetGlobalNumber + _RETN, + ] + ) + store_state = bytes([0x2C, 0x10]) + (0).to_bytes(4, "big") + (0).to_bytes(4, "big") + jmp_past_action = bytes([0x1D, 0x00]) + (6 + len(action_body)).to_bytes( + 4, "big", signed=True + ) + expected = _hand_assembled_ncs( + _JSR_MAIN, + _RETN, + store_state, # aActionToDelay, the last parameter, is pushed first + jmp_past_action, + action_body, + _constf(1.0), # fSeconds, the first parameter, ends up on top + _action(7, 2), # DelayCommand + _RETN, + ) + assert actual == expected, f"{actual.hex(' ')} != {expected.hex(' ')}" + # endregion # region Arithmetic Operator Tests