Fix NPC Vector3 argument marshalling in native proxies

[AgustinIbanez00]

Summary

Several NPC APIs that take a Vector3 input argument were unusable because of an ABI mismatch between the native proxy and the source-generated managed P/Invoke. Npc.SetPosition crashed the server (SIGSEGV), and path/aim/shoot APIs silently received garbage coordinates. The fix is a one-word change per affected proxy in src/sampsharp-component/proxies/api.cpp (native-only; no managed change required).

Affected APIs:

• INPC::setPosition (Npc.SetPosition) — crashed the server (SIGSEGV / exit 139).
• INPCComponent::addPointToPath (INpcService.AddPointToPath) — stored <0, 0, 0>, so an NPC driven with MoveByPath glided to the world origin instead of following the path.
• Same defect in hasPathPointInRange, shoot, aimAt, aimAtPlayer.

Closes #605

Problem

The managed side declares these methods with a by-value Vector3 parameter:

// SampSharp.OpenMp.Core – Api/Components/NPCs/INPC.cs
public partial void SetPosition(Vector3 position, bool immediateUpdate);
// Api/Components/NPCs/INPCComponent.cs
public partial bool AddPointToPath(int pathId, Vector3 position, float stopRange);

The source generator emits a P/Invoke that passes Vector3 by value (a blittable 12-byte struct: three floats). The native proxy, however, declared the parameter as const Vector3&:

// src/sampsharp-component/proxies/api.cpp (before)
PROXY(INPC, void, setPosition, const Vector3&, bool);
PROXY(INPCComponent, bool, addPointToPath, int, const Vector3&, float);

Root Cause

const Vector3& lowers to a pointer (Vector3*) in the exported extern "C" signature generated by the PROXY macro. The managed caller pushes 12 bytes of x, y, z where native expects an 8-byte pointer, and native dereferences those bytes as an address:

• setPosition → dereferences a bogus pointer → segfault.
• addPointToPath → reads garbage floats → stored point ends up ~<0, 0, 0>.

Proxies already declared by-value always worked, e.g. PROXY(INPC, bool, move, Vector3, ...) (Npc.MoveTo). By-value is the established convention across this boundary; the affected proxies were simply the inconsistent ones.

Solution

Change the affected input parameters from const Vector3& to Vector3 (by value) so the native signature matches the by-value managed marshalling. The open.mp SDK methods still take const Vector3&; a by-value Vector3 binds to a const Vector3& parameter, so the PROXY macro's subject->method(...) forwarding still compiles unchanged.

-PROXY(INPC, void, setPosition, const Vector3&, bool);
+PROXY(INPC, void, setPosition, Vector3, bool);
-PROXY(INPC, void, shoot, int, PlayerBulletHitType, uint8_t, const Vector3&, const Vector3&, bool, EntityCheckType);
+PROXY(INPC, void, shoot, int, PlayerBulletHitType, uint8_t, Vector3, Vector3, bool, EntityCheckType);
-PROXY(INPC, void, aimAt, const Vector3&, bool, int, bool, const Vector3&, EntityCheckType);
+PROXY(INPC, void, aimAt, Vector3, bool, int, bool, Vector3, EntityCheckType);
-PROXY(INPC, void, aimAtPlayer, IPlayer&, bool, int, bool, const Vector3&, const Vector3&, EntityCheckType);
+PROXY(INPC, void, aimAtPlayer, IPlayer&, bool, int, bool, Vector3, Vector3, EntityCheckType);
-PROXY(INPCComponent, bool, addPointToPath, int, const Vector3&, float);
+PROXY(INPCComponent, bool, addPointToPath, int, Vector3, float);
-PROXY(INPCComponent, bool, hasPathPointInRange, int, const Vector3&, float);
+PROXY(INPCComponent, bool, hasPathPointInRange, int, Vector3, float);

Intentionally unchanged

• Output parameters stay Vector3& — the managed side marshals these as out/ref (a pointer), which correctly matches a non-const reference (getPathPoint, getNodePointPosition).
• ICheckpointDataBase::setPosition and IRaceCheckpointData::setNextPosition also take a Vector3& input, but they are not affected: their managed declarations use ref Vector3 (which marshals as a pointer) and therefore already match the native Vector3&. They are left untouched.

Validation

Verified at runtime by driving NPCs in a downstream gamemode with the rebuilt SampSharp.so (stock managed 1.0.0-prerelease2 assemblies unchanged). Readback-based and reproducible:

1. SetPosition — spawn NPC (origin <0,0,3.5>), SetPosition(target, true), read Position:
   • Before: server crashes (exit 139).
   • After: reads back the exact target, e.g. SetPosition(<1260, -2342.25, 16>) → <1260, -2342.25, 16>.
2. AddPointToPath — CreatePath, AddPointToPath(pathId, p, range), GetPathPoint(pathId, 0, out stored, out _):
   • Before: stored == <0, 0, 0>.
   • After: stored == p exactly (e.g. <1260.875, -2335.375, 16.066>).
3. MoveByPath end-to-end — build a native path (hundreds of points), MoveByPath(pathId, Drive, speed, false):
   • Before: NPC glides straight toward <0, 0, 0>.
   • After: NPC drives the full route; OnNPCFinishMovePath fires; arrival within 1–2 m across multiple concurrent NPCs (~2.5 km routes).

Built with gcc/g++/cmake (RelWithDebInfo) per .github/workflows/component-linux.yml.

[ikkentim]

Thank you! I'll do some testing and merge this soon

[ikkentim]

One thing I'm slightly confused by is that the current state of the code matches what's in the open.mp SDK headers

https://github.com/openmultiplayer/open.mp-sdk/blob/master/include/Server/Components/NPCs/npcs.hpp#L38

[AgustinIbanez00]

Fair thing to flag — it does match the header exactly, and that's what threw me off at first too. The catch is that the types in a PROXY declaration aren't mirroring the SDK signature: the macro uses them verbatim as the parameter types of the exported extern "C" function that the managed P/Invoke calls into, so they define the ABI contract with C# rather than with the SDK. There const Vector3& lowers to a pointer, while the source generator passes the 12-byte struct by value — hence the mismatch.

What finally convinced me is that the SDK is inconsistent with itself here: move (L59) and setVelocity (L89) take Vector3 by value, while setPosition (L38), addPointToPath (L448) and hasPathPointInRange (L462) take const Vector3&. The proxies that mirrored the by-value ones have always worked; the broken ones are exactly the ones that mirrored the by-ref ones — 1:1, no exceptions. So the change lines the export up with what the managed side actually pushes, and the forwarding still compiles since a by-value Vector3 binds to const Vector3&.

Happy to write up repro steps if that helps when you test.