Ralith / hypermine

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

Fix inconsistencies with margin reconciliation logic #386

Closed patowen closed 6 months ago

patowen commented 6 months ago

This PR fixes two bugs related to https://github.com/Ralith/hypermine/pull/379.

Bug 1

Some of the logic assumed that world generation would never create "solid" chunks unless it was sure that the chunk was deep enough underground or high enough up in the air to not touch any surfaces. This assumption allowed us to avoid reconciling margins between a solid chunk and a dense chunk unless the dense chunk was "modified".

However, since this assumption was incorrect (for instance, due to world generation not turning solid "void" chunks into dense chunks unless a voxel is actually generated), this produced gaps in the terrain whenever a dense chunk met a solid void chunk.

Bug 2

When a voxel is modified, we assumed that only neighboring chunks' margins needs to update, since all other margins should already be correct. However, if we want to be certain, we also need to update the margins of the chunk the modified voxel is in. This is because we only guarantee that margin voxels are the correct material when they would be rendered.

Because of this, it would have been possible to dig down into a solid dirt chunk, dig across a bit, and dig up again, seeing what looks like dirt in a neighboring chunk's voxel no matter what material that voxel actually is.

Solution

The fix to bug 1 is to remove the assumption, adding logic to fix_margins to handle either chunk being solid.

The fix to bug 2 is to change update_margin_voxel into a bidirectional update, updating the margins of both relevant chunks. Since this changes the public interface of the function, I also renamed it to reconcile_margin_voxels.

Bonus cleanup

The only purpose of the modified boolean in Chunk::Populated was to determine whether assumptions based on world generation would hold when determining whether chunks should be solid or not. Since we no longer make assumptions based on world generation, this field is now obsolete, allowing us to remove it completely.

patowen commented 6 months ago

I believe the extra changes I made to this PR are noncontroversial enough, so I'll go ahead and merge when CI passes.