Restore world saving, fix five server crashes, and persist dropped items

[beleata]

Fourteen fixes, and one thing we could not finish

Hello. We are two people who wanted to play Hellion again, found OpenHellion, and
spent two days on it. We are not asking you to merge anything blind — everything
below sits in branches on our own forks so you can look first.

Everything here was found by actually playing: a local Nakama, the game server,
and the client built from your repository. One of us played and reported what he
saw; the other read logs and wrote code. Every fix below was reproduced in the
running game, not deduced from reading.

A question before anything else: are the three repositories on GitHub what you
are working from, or do you have newer local work? We branched from main on each
(Server at 84dda51, plus Client and Nakama at their current heads). If you
have unpushed changes, some of this may already be solved or may conflict, and we
would rather rebase than hand you noise.

---

Where it started

Persistence.Save builds the whole world, deletes old save files, logs
"Saved world..." — and writes nothing. The call to SerializeToFile has been
commented out with a TODO: FIX since c89a61f (2023-07-23).

So nothing has been saved for two years, nothing has been loaded, and the entire
load path has never run. Everything else we found was hiding behind that.

---

The fourteen

Server — saving and loading

1. World saving was disabled. Restored. The write also went to a path relative
to the working directory while Load and the save-file cleanup scan the directory
next to the assembly, so with -configdir set, saves landed where Load never
looks.

2. PersistenceJsonConverter.WriteJson recursed into itself through the same
JsonSerializer it is registered on. The concrete type is already written by
PersistenceData.__ObjectType, so writing needs no converter: CanWrite is now
false.

3. Loading ran in parallel over code that is not thread safe. Vessels were
loaded through Parallel.ForEachAsync, and the game code it drives mutates shared
state through plain List<T>. Docking corrupted the shared docked-vessels tree
into cycles, which overflowed the stack in BulletPhysicsController.RemoveRigidBody
and in OrbitParameters.RelativePosition; repair points corrupted
Room.AirConsumers, throwing out of List.Remove. Loading is now sequential.

We checked the save file itself first: 1566 vessels, 1222 with a docking parent,
zero cycles. The file was clean; the cycles were made on load.

4. Docking and stabilisation are resolved by GUID, so they now run in a second
pass once every vessel exists, rather than reaching for vessels not yet created.

5. RemoveRigidBody walked the docking tree recursively with no guard. It now
iterates with a visited set, so a malformed tree is reported instead of taking the
server down.

6. Room.AddAirConsumer accepted null, which surfaced much later as a
NullReferenceException in UpdateCompoundRooms — only in a depressurised room,
because a pressure check short-circuits the dereference one line earlier.

Server — five ways to kill the process

7. Every player death.QuestTriggerID's == operator dereferences both
sides, and killing a player compares it against every vessel in the world. Almost
no vessel has one. First comparison, exception, async void, server gone.
Its GetHashCode also built a multi-dimensional array out of the field values.

8. Destroying a vessel. Its docked vessels and crew went through
Parallel.ForEachAsync, and the spawn system and name generator it reaches into
keep plain dictionaries. Two vessels at once corrupt them.
InvalidOperationException, server gone.

9. A player floating with no vessel nearby.UpdateMovementListener
dereferenced nearestVessel before the guard against it six lines further down.

10. Reconnecting.GameTransport.DisconnectInternal ran its body twice for one
client and threw on the already disposed socket before the entry could be
removed — so the client stayed marked as connected and the server refused its next
login as a duplicate. The entry is now taken out first.

11. PlayerReady's setter compared with a single =. Its body ran on every
assignment instead of only on a change, firing whatever hangs off it many times a
second.

Client — two faults that stop a saved world from loading

12. Ship.FixedUpdate is async and awaits a network send. The continuation
can resume off the main thread, and reading Time.fixedDeltaTime after it throws.
The exception is never observed and loading simply stops. The value is now read
before the await.

13. QuestIndicators.IndicatorPrefab is not assigned in the scene. A restored
character has quest progress, so World.OnLogin places quest markers,
Instantiate throws out of the login handler, and the client sits on "Loading
World" for ever — with nothing in any log to say why. This one cost us an
evening. A missing prefab now skips the marker.

Nakama — the plugin does not load at all

14. Against a current Nakama, InitModule fails and the whole runtime refuses
to start: no game server can register, no client can find one.

• registerStorageIndex gained a sortableFields parameter before maxEntries,
  so the numeric maxEntries lands in the array position and the runtime rejects
  it with "expects an array of strings".
• storageIndexList now returns a result object with an objects array rather
  than a bare array.

Also: register_server records ctx.clientIp, and a server reaching Nakama through
anything that rewrites the source address gets stored under an address no client can
reach — with no way to correct it. GameServer.ini documents an ip_address
setting, but the server never sends one and the plugin never reads one. We added a
SERVER_IP_OVERRIDE runtime variable, falling back to ctx.clientIp as before.

---

Issue #3: items left lying in a vessel

Solved on the server. Not solved on the client — and we want to be exact about
which is which.

An item a player drops belongs to no vessel and to nobody's inventory: it rides on
its own Pivot. Nothing in Persistence could reach it, so every dropped item was
lost on shutdown. It is now recorded — which vessel it was released in, and where —
and put back on a pivot in that vessel on load.

Verified, repeatedly: the item is written to the save, survives a restart, is
restored, and is saved again. We watched the counts in the save files across
restarts.

What does not work: a joining client is told about these items but does not draw
them. The server thinks the item is ~0.2 m from the station's centre; the client
draws it ~25 m away, next to where it was dropped. Both are "right" in their own
frame, and nothing converts between them.

That is issue #1 — "fully embrace two coordinate space types" — in your own
words. We do not think it can be patched around, and we stopped rather than paper
over it.

---

What we tried and abandoned

We are listing these so nobody repeats them.

Parenting the dropped item to the vessel instead of a pivot. The item vanished
within a second. There is no representation for an item lying loose at an arbitrary
position inside a vessel: of 6983 item records in a world, every single one is
on an attach point. Only five had a free position, and those were ours.

Sending the item as a child of the vessel at spawn time. The client places it
by transform.localPosition under that vessel, which is a different frame from the
one the drop was measured in. Consistently ~25 m out.

Having the client fall back to the player's vessel when it cannot resolve the
pivot. It resolves, and the item lands at the station's origin.

Rebuilding the pivot on each join. This worked — 1.4 m from the player — but
it leaks an empty artificial body per join into the solar system's body list, which
is walked for every movement message; it makes the item jump for anyone already
watching it; and two players joining at once both rebuild the same pivot with no
locking. We took it out.

Pinning the pivot's orbit to the vessel every movement message. The pivot lands
at the station's centre, because the item's own offset lives in a frame we cannot
read.

Deriving the drop position from the server's own data. We tried the client's
attach position (a client-side root), the player's world position (returns the
vessel's own), and the player's LocalPosition (a different small-scale frame).
None of them is the item's offset within the station. The server does not know
where a dropped item is, in any frame it can reason about — dropped items are
client-authoritative.

That last line is the real finding, and it is why we stopped.

---

The branches

Repository	Branch	What is in it
beleata/Server	fix/persistence-not-saved	fixes 1–11, plus issue #3 persistence
beleata/Client	fix/loading-a-saved-world	fixes 12–13
beleata/Nakama	fix/load-against-current-nakama	fix 14

Two commits on the server: the first restores saving and the crashes it exposed,
the second the five process-killers and the dropped-item persistence.

Take whatever is useful, in whatever shape suits you — squash it, rewrite it,
or just take the diagnosis and write it yourself. We are glad the game is still
alive, and we would rather it stayed that way than get credited for anything.

Two smaller things we did not touch

• OrbitParameters.cs:71 has a Debug.LogFormat inside a property getter that is
  read constantly. It produced ~16,000 log lines in two minutes while a world
  loaded.
• -shutdown cannot start while a server is running from the same directory:
  Debug.Initialize() holds output_log.txt exclusively, so the shutdown helper
  dies before it can send the message.

One question about the repository itself

Server has no licence file, while Client and Nakama are GPL-3.0. That leaves
contributions there in an awkward spot. Would you consider adding one?

[Makkkkus]

Sorry for not replying earlier! Got back to this now when the weekend started. I really appreciate you both taking the time to test and edit the codebase.

I especially appreciate you looking into saving and loading as it has been a bother for me for some while.

I currently have a local working tree with over eight months of changes for a separation of the world into local and solar system space (#1). In doing this I have made an immense number of changes: touching 418 files on the client and 125 files on the server, totalling in thousands of lines of edits. I regret not pushing this so you could look at it.

I will am very close to finishing most of the remaining bugs from the rewrite. I will push the changes at the end of this weekend, even if they are incomplete. I will add a .todo file if they still are broken.

[beleata]

Thank you for the reply, and no rush at all — eight months of work on a world-space
separation is a far bigger thing than anything we sent.

Take these whenever it suits you, and only if you think they fit. We are not
asking for them to go in as they are. If it is easier for you to land your rewrite
first and let us rebase onto it afterwards, say so and we will do that instead.

The one reason we mention ordering at all: all fourteen are small and independent of
each other, and none of them touch world-space geometry, so they should sit under
your rewrite rather than across it. If they go in first, your tree carries them and
you do not have to think about them again.

What we changed, and why

Everything traces back to one thing. Persistence.Save builds the whole world,
deletes the old save files, logs "Saved world..." — and writes nothing. The call
to SerializeToFile has been commented out with a TODO: FIX since c89a61f, July
2023. So for two years nothing has been saved, nothing has been loaded, and the
entire load path had never once run. Every other fault we found was hiding behind
that, which is why they come as a group.

Server, saving and loading. Saving restored, and the write pointed at the same
directory Load reads from — with -configdir set they were different, so saves
landed where loading never looks. PersistenceJsonConverter.WriteJson recursed into
itself through the serialiser it is registered on; the concrete type is already
written as PersistenceData.__ObjectType, so CanWrite is now false. Loading ran
in parallel over code that is not thread safe. Docking and stabilisation resolve by
GUID, so they now run in a second pass once every vessel exists.
RemoveRigidBody walked the docking tree recursively with no guard.
Room.AddAirConsumer accepted null and the consequence surfaced much later,
somewhere else.

Server, five ways to kill the process. Every player death, through
QuestTriggerID's == operator dereferencing both sides. Destroying a vessel,
through its docked vessels and crew. A player floating with no vessel nearby.
Reconnecting, because DisconnectInternal ran its body twice for one disconnect.
And PlayerReady's setter comparing with a single =, so its body ran on every
assignment.

Client, two faults that stop a loaded world from ever finishing.
Ship.FixedUpdate is async and awaits a network send; the continuation can resume
off the main thread and reading Time.fixedDeltaTime afterwards throws, unobserved,
and loading simply stops. And QuestIndicators.IndicatorPrefab is not assigned in
the scene — a restored character has quest progress, Instantiate throws out of the
login handler, and the client sits on "Loading World" for ever with nothing in any
log to explain it.

Nakama. The plugin does not load at all against a current Nakama:
registerStorageIndex gained a sortableFields parameter before maxEntries, and
storageIndexList now returns an object with an objects array rather than a bare
array. Without those, InitModule throws, the runtime refuses to start, and no
server can register.

All of it was found by playing rather than by reading: one of us plays and reports
what he sees, the other reads logs and writes the code. Nothing above was deduced
without being reproduced in a running game first.

A question

Do you know about the Hellion Revival Project — Zorkind's work, at hellionrp.space?
It goes at the same game from the opposite end: no source at all, patching the
shipped client with asset bundles and replacing whole modules that way. Their
changelog lists well over a hundred bug fixes, and a fair number of them read like
the same faults we have both been chasing.

I ask because it seems a waste for two efforts to solve the same game twice without
knowing of each other. Have you had any contact with them, and would you want any?

And one practical thing: is there a Discord, or anywhere else you would rather talk?
Pull request comments are a slow way to work something out together — a day between
each turn — and some of what we are running into would take two minutes to settle in
a chat. Happy to go wherever suits you.

[Makkkkus]

I have looked briefly through your changes. All of them seem good and pretty self-explainatory. I appreciate you taking the time to look at this.

I am familiar with Zorkind and his revival project; I have written a bit about the differences between the two projects on our about page. I have talked a bit to Zorkind on his Discord server, and I am not very impressed with how things are run there.

Hellion Rescue Project (HRP) is essensially a dictatorship with him having complete control over all aspects of the project. He argues that is is the only correct way to do things because it is "illegal" to distribute the original game; while ignoring that he himself distributes modified binaries of the original server, which is just as illegal.

Zorkind has created a fake impression that our project is illegal, when it is not. I think the reason why Zorkind keeps repeating this is that he wants control (corroborated in that he has spread fake stories of contacting the original owners offering to buy the game many times and falsely claimed to have talked to me about the legality of our project). The main reason why this project exists is to let new players access Hellion, which is not possible with HRP's approach.

We have a Discord! The link is: https://discord.gg/9nGWgQ8Uyf

[beleata]

We were wrong about dropped items, and I have pushed the correction. The pivot
persistence in this PR is gone, along with everything that propped it up: the
change is now 88 lines added against 158 removed.

What is actually broken

A client whose item is resting on the floor of a vessel reports that the item has
left the vessel about once a second, forever. Its room trigger fires an exit
every time the item is re-parented, and the reply re-parents it again, so the two
chase each other for as long as the item exists.

Believing it puts the item on a pivot of its own. A pivot's contents are described
exactly once — in the message announcing the drop — because SolarSystem walks
each vessel's dynamic objects when it builds the movement message, and a pivot is
not a vessel. From that moment the item is invisible to anyone who arrives later,
including the player who dropped it, once they reconnect. And since it belongs to no
vessel, there is nothing under any vessel to write down when the world is saved.

That is why our first attempt looked like a persistence problem. It never was.

Why nobody has hit this before

The shipped game destroys a floating item five minutes after anyone last touched it,
and never saved them. Nobody could see that they had already stopped existing for
everyone else.

How we measured it

We ran the identical action against two servers, logging every attach message:

retail client, 2018 server:
parent Player, attached True <- picked up
parent Ship, attached False <- dropped
(nothing further)
OpenHellion client:
parent Player, attached True
parent Ship, attached False
parent DynamicObjectPivot <- and again, twice a second, forever

The client code along that whole path — TransitionTriggerHelper, EnterVessel,
ExitVessel, AreAttachDataSame, ProcessAttachData, Pivot.Create — is
identical between the two, line for line. So is DynamicsManager.asset. The
difference is in behaviour, not in source, and we have not found where yet. It is
worth finding: every turn of that loop re-parents a GameObject and re-runs its
trigger and collider checks on a client that is already CPU bound. Fifty items lying
about a station is a hundred of those a second for nothing.

What the commit does

Four changes, all on the server, none of which depend on finding that client fault:

1. A release inside a vessel leaves the item owned by that vessel, and the reply
   carries that back to the client. The client may keep claiming otherwise; the
   server knows better and the claim decides nothing. Released in open space it
   still goes on a pivot, and the five minute cleanup still applies there.
2. An object with no master client was described to nobody.
   PlayerReceivesMovementMessage read playerGuid != MasterClientID && MasterClientID != 0. An object restored from a save has never been touched, so
   it has no master, and that test answers "no" for everyone. With no master the
   server is the only authority there is.
3. A player who has just connected gives up whatever they were the master of.
   Their new client knows nothing, and while it held that title the server would not
   tell it where those objects were — which is why a reconnect lost them even
   without a restart.
4. A pivot's dynamic objects are walked when building the movement message, the
   same as a vessel's, so a tool someone drops in open space beside you is not
   invisible.

Tested: drop, watch, reconnect, restart the server, reconnect again. The items are
where they were left, on the floor, and can be picked up. The save now holds them as
the vessel's own contents, exactly as a 2018 save does.

One thing we noticed and did not chase

Picking a restored item up gave the wrong item: a repair tool came back as a stim
pack, was consumed as one, and the medpack that had been lying beside it vanished
from the world. The server's records were right throughout — the repair tool was on
the player, with the right item id. It looks like the client associating spawn data
with the wrong object, and it is probably the same family of fault. We have not
looked into it.

A question about open space

Anything let go of outside a vessel is destroyed five minutes after it was last
touched. That is the shipped rule and we have left it alone, but on a server that
runs for months it may deserve to be a setting rather than a constant — five minutes
is unforgiving if somebody fumbles a tool during a repair, and forever would be
worse. Your call.

[Makkkkus]

Picked the persistence findings and commited it as 7613843. The other fixes might need to be looked into later.

Closing this as done.