Ralith / hypermine

A hyperbolic voxel game
Apache License 2.0
160 stars 20 forks source link

In-process server #425

Closed Ralith closed 3 months ago

Ralith commented 3 months ago

Towards https://github.com/Ralith/hypermine/issues/405.

Rough in some places ~and not actually working correctly yet~.

patowen commented 3 months ago

I wanted to provide this comment mainly out of curiosity:

Weirdly, when I tried to compile this PR, I got

error[E0282]: type annotations needed for `Box<_>`
  --> C:\Users\Patrick\.cargo\registry\src\index.crates.io-6f17d22bba15001f\time-0.3.28\src\format_description\parse\mod.rs:83:9
   |
83 |     let items = format_items
   |         ^^^^^
...
86 |     Ok(items.into())
   |              ---- type must be known at this point
   |
help: consider giving `items` an explicit type, where the placeholders `_` are specified
   |
83 |     let items: Box<_> = format_items
   |              ++++++++

I resolved this by deleting Cargo.lock, so if anyone is interested in investigating what semver issue lead to this error, I've attached the Cargo.lock file that caused the error:

Cargo.lock.zip

It's probably not worthwhile debugging, though. It seems to be entirely unrelated to this PR and likely instead related to changes to my setup from hopping around various PRs.

Ralith commented 3 months ago

Probably a minor issue in an upstream crate that got fixed.

patowen commented 3 months ago

This improved efficiency seems to have revealed a subtle bug I introduced in https://github.com/Ralith/hypermine/pull/414/files#diff-e8d8336a10bad73657026c14d2e141ca92d15c95a92aa7ca93df5366de21754eR186.

Normally, the server's graph is kept in a clean state. Whenever nodes are added to it, these nodes are processed fully before any other code gets a chance to run. See code snippet. This generally creates a guarantee that if snapshot is called (for transmitting info to new clients), and step is called afterwards, the Spawns returned by step will never contain any graph nodes that were already returned by snapshot.

Before a server's first step, its state is polluted. Calls to ensure_neighbor are invoked without an associated populate_fresh_nodes. This means that when snapshot is called, these nodes are included as if they're already initialized. However, step still views these nodes as new/uninitialized, so it also includes them in the Spawns it returns.

The effect is that if a client connects to a server before the server runs its first step, then as long as there are any players outside of the root node that need to be loaded, the server will send a client the same node twice, resulting in a crash.

The simplest workaround is to add result.step(); right before result is returned in Sim::new, guaranteeing that the first step will complete before anything else.

However, a longer-term fix would be to ensure that server::Sim never reaches this invalid state to begin with. I'm open to ideas on how to do this. Long-term, I wonder if we want to stop Graph from storing its own fresh nodes, since that feels like a potential foot-gun.

EDIT: This should be fixed by https://github.com/Ralith/hypermine/pull/427.

Ralith commented 3 months ago

Long-term, I wonder if we want to stop Graph from storing its own fresh nodes, since that feels like a potential foot-gun.

Our plan has always been to do away with the "fresh nodes" paradigm entirely in favor of deterministic node IDs, right?

patowen commented 3 months ago

Our plan has always been to do away with the "fresh nodes" paradigm entirely in favor of deterministic node IDs, right?

I believe so, although it's unclear to me what that will look like. It probably makes sense to figure that out before deciding where to store them.