fix: Multiplayer nil-reference error, global variable leak, network serialization safety, and XMLFile handle leak

[XelaNull]

Summary

Companion PR to our translations contribution. While working through your code for that, we spotted a few bugs — mostly multiplayer-related — and wanted to help fix them.

Full disclosure: These fixes were developed with AI assistance (Claude Code by Anthropic). A human developer reviewed and verified each change. Every fix was identified through line-by-line code review against the source. Singleplayer loading was verified (no Lua errors), but multiplayer-specific behavior was not runtime-tested.

4 targeted fixes, each with detailed inline comments explaining the why behind the change. No functionality changes, no refactoring, no style opinions — just bug fixes.

• Fix 1: Multiplayer client nil-reference error when changing decimal settings (MinGreed, MaxGreed, etc.)
• Fix 2: Global variable leak in onWriteStream / onReadStream
• Fix 3: Network serialization made order-independent for custom map compatibility
• Fix 4: XMLFile handle leak on every game save

---

Fix 1: Multiplayer Client Error (changeDFPDecimalSettingsEvent.lua)

The bug: When any player changes a decimal setting (e.g., MinGreed slider) in multiplayer, all OTHER clients hit a nil-reference error. We identified this through code review — we haven't tested the actual multiplayer behavior, so we can't say for certain whether this causes a client disconnect, a logged error, or a crash. But the code path is clear:

The error path (from code review):

1. Client A changes MinGreed slider
2. Client A sends ChangeDFPDecimalSettingsEvent to server
3. Server's run() executes:
   → Updates setting ✅
   → Calls calcPrice() ✅ (server has g_server)
   → Broadcasts event to Client B, C, etc. ✅
4. Client B receives event, run() executes:
   → Updates setting ✅
   → Calls calcPrice() ← PROBLEM
     → calcPrice() line 78: g_server:broadcastEvent(...)
     → g_server is nil on clients → Lua nil-reference error

Why it's wrong regardless of severity: Even if the GIANTS engine catches this error gracefully, calcPrice() on clients is redundant work — clients already receive updated prices via DFPPricesChangedEvent (which calcPrice() broadcasts from the server). The call shouldn't be there.

The fix: Move calcPrice() inside the g_server ~= nil guard. This also matches the pattern already used in ChangeDFPCheckSettingsEvent:run(), which correctly does NOT call calcPrice().

Note: Your mod already has the perfect solution — DynamicFieldPrices:onSettingsChanged() (line 110) wraps calcPrice() in an isServer guard. It's just never called!

---

Fix 2: Global Variable Leak (DynamicFieldPrices.lua)

The bug: Lines 28 and 35 assign fls = self.farmlandManager.farmlands without the local keyword, creating a global variable. Lines 43 and 52 in the same file correctly use local fls — so this is clearly an oversight, not a style choice.

The risk: Any other mod using a global named fls would collide. In singleplayer this is harmless in practice, but it's a one-word fix.

---

Fix 3: Network Serialization Safety (DynamicFieldPrices.lua + DFPPricesChangedEvent.lua)

The issue: The network code writes and reads farmland prices using pairs() iteration, which does not guarantee order in Lua. Additionally, DFPPricesChangedEvent uses the # (length) operator on the prices table, which returns undefined results for tables with non-sequential keys.

When it matters: On vanilla FS25 maps, farmland IDs are contiguous (1, 2, 3, ..., N), so pairs() happens to iterate in order and # returns the correct count. But on custom maps where farmland IDs might have gaps (e.g., {1, 2, 5, 10}), prices could be assigned to wrong fields or the stream could desync.

The fix: Tag each price with its farmland ID in the stream. Instead of writing prices in whatever order pairs() gives us and hoping the reader iterates in the same order, we write (farmlandId, price) pairs. The reader maps each price back to the correct farmland by ID. This is order-independent and works regardless of farmland ID layout.

Bandwidth impact: Adds one int32 per farmland per sync (about 320 extra bytes on an 80-field map). Negligible — this only fires once on player join and once per in-game day.

---

Fix 4: XMLFile Handle Leak (DFPSettings.lua)

The bug: saveSettingsXML() creates an XMLFile handle with XMLFile.create() and calls xmlFile:save(), but never calls xmlFile:delete() to release the handle. The loadSettingsXML() function at line 193 correctly calls xmlFile:delete() — the save function just missed it.

The risk: Each game save (manual or auto) leaks one XML handle. Over a long play session this accumulates. One-line fix.

---

What We Didn't Change

We intentionally left these alone — they're code style choices, not bugs:

• Semicolons in event files (style preference)
• print("Day passed!") debug statements (your call whether to gate these behind DFPSettings.debug)
• German comments (totally fine, it's your codebase)
• The updateSetting global function in DFPSettings.lua (it works correctly despite missing local — each closure captures the right values at assignment time)

---

We've also submitted a separate translations PR adding all 21 missing languages (bringing you to 26/26 complete coverage).

— Claude & Samantha
🤖 Generated with Claude Code (Claude is the AI, Samantha is our AI project-manager persona) — reviewed and verified by a human developer