Refactor graph expansion to remove the concept of "fresh" nodes #453
Conversation
This is instead of waiting for the modified chunk in question to be added to the server's Graph data structure
With the current level of optimization in Hypermine, it is not possible to simultaneously avoid pop-in, avoid unacceptable lag, and avoid unacceptably thick fog. To get around this issue, we make allowing pop-in explicit in the configuration
Switch from an `Option<Node>` with a `NodeState` to a `Node` with an `Option<NodeState>`. This reduces the number of unwraps or error handling needed by code that doesn't care about `NodeState`.
patowen
force-pushed
the
refactor-graph-expansion
branch
from
a5e2e92 to
891e2cc
Compare
patowen
force-pushed
the
refactor-graph-expansion
branch
from
891e2cc to
98b23c5
Compare
Tangential, but I wonder if we can find a better balance by using nonphysical fog that thickens rapidly but has a distant start point... |
common/benches/bench.rs
Outdated
| @@ -39,7 +39,7 @@ fn build_graph(c: &mut Criterion) { | |||
| c.bench_function("worldgen", |b| { | |||
| b.iter(|| { | |||
| let mut graph = Graph::new(12); | |||
| ensure_nearby(&mut graph, &Position::origin(), 3.0); | |||
| ensure_nearby(&mut graph, &Position::origin(), 2.25); | |||
There was a problem hiding this comment.
What's the intent of this change?
There was a problem hiding this comment.
ensure_nearby was modified to return more nodes for a given distance, so this benchmark needed to be adjusted accordingly.
| pub view_distance: Option<f32>, | ||
| /// Maximum distance at which new chunks will be generated in meters | ||
| pub chunk_generation_distance: Option<f32>, | ||
| /// Maximum distance at which fog becomes completely opaque in meters |
There was a problem hiding this comment.
s/Maximum //. The distance that fog becomes opaque at is a single value, not a range.
|
|
||
| /// Initializes the NodeState for the given node and all its ancestors if not | ||
| /// already initialized. | ||
| pub fn ensure_node_state(&mut self, node_id: NodeId) { |
There was a problem hiding this comment.
We often seem to need to access the state immediately after ensuring it. Would that code be more concise if this function returned the &mut NodeState directly?
There was a problem hiding this comment.
It should only need to return a &NodeState, but otherwise, that seems like a good suggestion to me.
| ) { | ||
| graph.ensure_node_state(node_id); | ||
| } | ||
| let local_to_view = view.local.inverse(); |
There was a problem hiding this comment.
nit: let's try to adopt a convention of x_from_y when naming transforms. This makes chains much easier to read and sanity-check because it matches the multiplication order: x_from_y * y_from_z is a lot more obviously correct than y_to_x * z_to_y.
There was a problem hiding this comment.
Oh, that's an interesting approach. Maybe in a separate PR? The dodeca module has a lot of functions we would also want to change.
| let Some(params) = | ||
| common::worldgen::ChunkParams::new(graph.layout().dimension(), graph, chunk_id) | ||
| else { | ||
| // Skip chunks beyond the chunk generation distance |
There was a problem hiding this comment.
Does nearby_nodes(..., chunk_generation_distance) already accomplish this? If it's not intended to, explain in a comment.
There was a problem hiding this comment.
nearby_nodes only returns nodes. We would need a nearby_chunks function to be as precise as this.
| let state = graph[chunk.node].state.as_ref()?; | ||
| Some(Self { | ||
| /// Extract data necessary to generate a chunk, generating new graph nodes if necessary | ||
| pub fn new(dimension: u8, graph: &mut Graph, chunk: ChunkId) -> Self { |
There was a problem hiding this comment.
Can we remove the dimension argument in favor of fetching it from the graph directly?
There was a problem hiding this comment.
I had thought of that but also thought about leaving it for a future refactor. However, since I'm touching this code anyway, might as well.
Possibly. I'd have to try it out. I do plan on trying to better optimize worldgen in the future, and I'll have to double-check that we're not including too many chunks the user wouldn't see past the fog. |
Note: The first few (6 as of the time I'm writing this) commits match #452, so this PR effectively branches off of #452, and only the later commits should be reviewed.
Since keeping the server and client graphs entirely in sync is one of the main reasons
Graphneeded to keep track of its freshly created nodes, and now they no longer need to be in sync this way, this PR removes this functionality to simplifyGraph's usage. Specifically, eachNodeis initialized immediately when it's added to the graph instead of being initialized toNone. TheNodeStatefield, which is used in fewer places, is still kept optional, and instead of being populated for every fresh node, it is populated for a givenNodeIdwith anensure_node_statefunction.Part of why
NodeStateis kept optional is that its initialization order will need to be more complicated for megastructures, although there may be a way in the future to make things simpler on that front as well.Since this PR makes heavy changes to graph expansion, it also addresses most of #50 by making the "distance" computations correct. However, with the current level of optimization, it is impossible to avoid the following three problems: unacceptably thick fog, pop-in, and unacceptable performance issues. Therefore, we still allow pop-in by making fog distance, view distance, and chunk generation distance separate settings with different defaults in the config.