fix(ncs): push engine call arguments last-first (#351)

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()

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

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