Expand world generation and optimize chunk streaming

[MichaelFisher1997]

Summary

• expand deterministic world generation with versioned terrain, caves, hydrology, ores, structures, POIs, and biome variants
• add durable multi-world storage and a Play hub for continuing, loading, and deleting worlds
• improve full-detail streaming, collision prioritization, edit responsiveness, and opt-in balanced LOD
• shard and extend CI coverage for worldgen, storage, streaming, and compatibility

Verification

• redot --editor --headless --path . --quit
• complete local equivalents of all PR test shards
• Parse and fast checks: 13/13 passed
• Worldgen core: 7/7 passed
• Worldgen regression: 6/6 passed
• Streaming: 6/6 passed
• git diff --check origin/main...HEAD

[github-actions]

I have completed a comprehensive review of all 66 changed files, tracing the full files behind the diffs (voxel_world.gd, world_storage.gd, terrain_sampler.gd, voxel_populator.gd, chunk_mesher.gd, player.gd, main.gd, play_panel.gd, and the catalogs) and cross-checking the streaming, edit-staging, worker-safety, and determinism invariants.

📋 Summary

No linked issues ("Fixes #…") are referenced in the PR description; this is a standalone feature/audit PR.

This PR lands three large workstreams in one commit: (1) versioned worldgen expansion (worldgen v9→v13 with legacy compatibility paths, ore/structure catalogs, climate variants, routed elevated hydrology, region POIs), (2) durable multi-world persistence (WorldStorage ZSTD region files + Play hub UI + autosave/exit flush), and (3) streaming responsiveness (ring-based desired enumeration, collision-priority scheduling with one bounded urgent slot, selective single-edit light invalidation, movement sweep/collision-readiness guards in Player). The engineering quality is high: legacy output paths are genuinely gated and fixture-pinned, worker-thread discipline is maintained (all new scratch is call-local, I/O stays on the main thread), and the PR ships 14 new headless verifiers sharded into CI. The main residual risks are one silent-failure path in persistence and a couple of narrow visual-seam edge cases.

📌 Review Metadata

• Reviewed Commit SHA: a729b89e68e70d3ddfa19e5d635cf61b018865d6
• Reviewed PR: Expand world generation and optimize chunk streaming #21
• Previous automated reviews: none found — nothing to re-verify or acknowledge as fixed.

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified. Specifically verified clean:

• Edit staging: every _record_edit path (break_block/place_block via _touch_chunk → _stage_chunk_edits, carve_sphere loop, _water_tick/_flush_fire_changes loops) stages to the store before an unload can discard the bucket, so no edit-loss path exists under healthy I/O.
• Worker safety: DecorationGroundScratch/TreeCandidates are per-populate() call, OreCatalog/StructureCatalog are immutable, region hydration/decode happen on the main thread, and _chunk_data_snapshot() only hands duplicates to workers.
• _schedule_jobs overflow is bounded to exactly one urgent job past _max_active_jobs (voxel_world.gd:534 breaks the loop once pending exceeds the cap).
• The _compute_block_light rewrite (per-cell scan instead of per-id find) preserves the max-based BFS fixed point, so results remain order-independent and deterministic.

⚠️ High Priority Issues (Should Fix)

None identified. Items I investigated and cleared:

• _touch_chunk's selective neighbor invalidation (voxel_world.gd:1893) is sound for opaque↔opaque, like-attenuation, water-level, emission-color, and boundary-topology cases; LIGHT_MAX_PROPAGATION_DISTANCE matches BFS lateral attenuation, and _block_touches_chunk_boundary correctly covers diagonal AO corner columns. Covered by tools/light_invalidation_verify.gd.
• Commit/version/mode staleness checks survive the new unload-discard paths (_discard_unloaded_chunk_state defers while _pending/_commit_queue entries reference the chunk).
• Region codec run-merging writes/reads symmetric ((y,z,x) sort equals local-index order; 65535 run cap enforced on both sides).

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] game/main.gd:329-337 - Silent persistence failure keeps a broken store attached and then discards in-memory edits
Confidence: Medium
Description: _prepare_world_storage() ignores create_world()/open_world() failure and attaches the failed WorldStorage anyway; _flush_world_save() (game/main.gd:380) also swallows flush() errors. When user:// is unwritable or a region write fails persistently, _discard_unloaded_chunk_state() (world/voxel_world.gd:1183) still treats the store as authoritative and erases _edits_by_chunk/_edited_blocks on unload — but nothing ever reached disk, so those edits are silently lost. The player also gets no feedback that autosave is failing.
Impact: Silent loss of player edits and session progress in low-disk/permission-failure situations; a corrupt-open can also silently fork a "new" world with the same config.
Suggested Fix: Fall back to in-memory mode when the store cannot be created/opened, and surface flush failures:

func _prepare_world_storage() -> Dictionary:
	_world_storage = WorldStorage.new()
	var metadata: Dictionary = {}
	if GameConfig.has_active_world():
		metadata = _world_storage.open_world(GameConfig.active_world_id)
	if metadata.is_empty():
		metadata = _world_storage.create_world(GameConfig.world)
	if metadata.is_empty():
		_world_storage = null  # keep pure in-memory behavior; edits survive unload
		return {}
	GameConfig.activate_world(metadata)
	return (metadata.get("state", {}) as Dictionary).duplicate(true)

and in _flush_world_save() emit set_status("Save failed") (or similar) when the flush returns non-OK.

ℹ️ Low Priority Suggestions (Optional)

[LOW] world/voxel_world.gd:1926-1933 - Cross-block edits at a chunk boundary skip neighbor water-face re-culling
Confidence: Low
Description: _single_edit_can_change_boundary_visibility excludes FLAG_CROSS blocks, but _append_water_block (world/chunk_mesher.gd:1125) culls water side faces against cross neighbors. Breaking a natural non-emissive cross plant in a boundary column next to level-1 flowing water in the adjacent chunk leaves that neighbor's water face permanently culled (a see-through gap), because the water tick's feed is 0 and no ring rebuild is triggered. Most variants self-heal (torch is emissive → light rule; source/lateral water flows in and triggers the full ring rebuild).
Impact: Rare one-block visual seam in water at chunk borders.
Suggested Fix: In _single_edit_can_change_boundary_visibility, also return true when exactly one side is a cross block:

	if _is_cross_id(old_block_id) != _is_cross_id(new_block_id):
		return true

[LOW] ui/main_menu.gd:262-266 - Creating a world is committed to disk at menu time
Confidence: High
Description: _on_create_world calls storage.create_world(...) before _start_game(); backing out afterwards (Esc during load, crash, or simply returning to menu) leaves an empty orphan world permanently listed in Load World.
Impact: Library clutter; users must manually delete ghost worlds.
Suggested Fix: Either defer create_world to Main._prepare_world_storage() (pass the requested id/config through GameConfig), or prune zero-edit, never-entered worlds during list_world_summaries.

[LOW] world/voxel_world.gd:191-192 - No-op ternary in decompress_chunk_data
Confidence: High
Description: return PackedByteArray() if palette_size == 0 and compressed.size() == offset else PackedByteArray() returns an empty array on both branches, which obscures the intended "empty-input is valid" case.
Impact: None functionally; readability only.
Suggested Fix: Replace with a plain return PackedByteArray() (and drop the unreachable condition), or validate and comment the empty-payload case.

[LOW] autoload/game_config.gd:217-218 - Dead API
Confidence: High
Description: latest_saved_world() is defined but no caller uses it (the play panel calls WorldStorage.latest_world_metadata() directly).
Impact: Slight API drift risk.
Suggested Fix: Remove it or route PlayPanel._refresh_landing() through it.

[LOW] player/player.gd:176-178 - Collision-wait freeze also stops targeting/footsteps
Confidence: Medium
Description: The early return when is_collision_ready_at() is false skips _update_target() and _update_footsteps(), so the crosshair/audio freeze for the few frames a pending collision rebuild takes. The movement pause itself is the documented design (AGENTS.md).
Impact: Minor perceptible hitch during extreme-streaming catch-up.
Suggested Fix: Move _update_target() (and optionally footsteps) above the early return.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	WorldStorage, OreCatalog, StructureCatalog, and the Play hub each own one concern; mesher/world/populator boundaries respected
Open/Closed	8	Version-gated catalogs and WorldGenConfig ranges extend behavior without editing legacy paths; new settings keys added to all presets per convention
Liskov Substitution	7	NeighborSample.from_lod honors the base contract; no inheritance misuse; mostly procedural code so little substitution surface
Interface Segregation	7	Player↔Main still signals-only; VoxelWorld exposes narrow query APIs (is_collision_ready_at, loaded_motion_fraction) instead of internals
Dependency Inversion	6	GameConfig/Main depend on the WorldStorage concrete class (fine for GDScript scale); the injectable _library_root in PlayPanel is a good testing seam
Average	7.2

🎯 Final Assessment

Overall Confidence Score: 84%

Confidence Breakdown:

• Code Quality: 88% (careful invariants, thorough comments, version-gated compatibility; minor dead code)
• Completeness: 86% (roadmap items marked done are genuinely implemented; world-management extras honestly left open)
• Risk Level: 82% (large blast radius, but stale-job/version/mode guards and bounded overflow slot hold; residual risk in silent I/O failure)
• Verification: 82% (14 new verifiers + sharded CI and the PR reports all shards passing locally; the engine binary is unavailable on this runner so none were re-executed here — treat as residual risk, not a blocker)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns (path traversal guarded by _safe_id, atomic temp/backup writes, no secrets)
• No unresolved worker-thread, determinism, or data-loss risk

Verdict:

MERGE WITH FIXES — mergeable once the silent persistence-failure fallback (Medium) gets a guard; everything else is polish.

Machine Readable Verdict

{
  "reviewed_sha": "a729b89e68e70d3ddfa19e5d635cf61b018865d6",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 1,
  "overall_confidence_score": 84,
  "recommendation": "MERGE WITH FIXES"
}

opencode session  |  github run

[MichaelFisher1997]

Addressed the automated review in db4ba71:

• fall back to in-memory world state if storage setup fails and surface autosave failures to the player
• defer new-world creation until the gameplay scene has loaded
• invalidate neighboring water meshes when boundary cross blocks change
• keep targeting active during collision waits
• remove the dead saved-world helper and simplify the empty codec path

Affected parse/UI/player/storage/light/codec checks pass locally.

[github-actions]

All checks complete. The fix commit is verified end-to-end against the full files.

📋 Summary

No linked issues ("Fixes #…", "Closes #…") are referenced in the PR description; this remains a standalone feature/audit PR combining worldgen v9→v13, durable multi-world persistence, and streaming responsiveness improvements.

Since the prior review, commit db4ba71b ("Address persistence review feedback") was added, which resolves the previously reported Medium issue and all five Low issues. The fixes are correct and well-integrated: the persistence fallback properly degrades to in-memory mode with null-store safety verified across VoxelWorld's edit lifecycle, world creation is correctly deferred to the gameplay scene, and the cross-block boundary invalidation rule is correctly ordered within the existing attenuation/water-level predicate chain.

📌 Review Metadata

• Reviewed Commit SHA: db4ba71bfdee59b7139ef8ec4612ffe0036b02ef
• Reviewed PR: Expand world generation and optimize chunk streaming #21
• Previous automated reviews: 1 (at a729b89e); all six reported issues verified below.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved (none were reported; the areas the prior review verified clean — edit staging, worker safety, bounded urgent slot, deterministic block-light BFS — are untouched by the fix commit, which only modified six files).

None identified.

⚠️ High Priority Issues (Should Fix)

✅ All previously reported high priority issues have been resolved (none were reported; _touch_chunk invalidation logic, commit staleness guards, and the region codec are unchanged except for the intended cross-rule and empty-payload simplification).

None identified.

💡 Medium Priority Issues (Nice to Fix)

✅ [FIXED] Previous issue: silent persistence failure kept a broken store attached and discarded in-memory edits.
Verified across the full call graph:

• game/main.gd:336-340 — _prepare_world_storage() now falls back to _world_storage = null + GameConfig.clear_active_world() + warning when both open_world() and create_world() fail, exactly matching the suggested remedy.
• game/main.gd:385-395 — _flush_world_save() chains world.flush_edit_store() (returns Error) into _world_storage.flush() (also Error, so the := inference and reassignment are type-safe) and surfaces failure with push_warning + set_status("Save failed - progress remains in memory").
• Null-store safety: Main._ready() calls _prepare_world_storage() (main.gd:76) before world.set_edit_store(_world_storage) (main.gd:80); VoxelWorld.flush_edit_store() returns OK on null (voxel_world.gd:296-299), _discard_unloaded_chunk_state() retains in-memory _edits_by_chunk/_edited_blocks when the store is null (voxel_world.gd:1183-1184), and _hydrate_chunk_edits()/_stage_chunk_edits() no-op safely (voxel_world.gd:1761-1762, 1772-1773). The set_status call from the _exit_tree failure path is safe — _status_label is still a valid object and Motion.fade_in is validity-guarded (ui/motion.gd:82-83).

None identified.

ℹ️ Low Priority Suggestions (Optional)

✅ [FIXED] Previous issue: cross-block edits at a chunk boundary skipped neighbor water-face re-culling. voxel_world.gd:1933-1934 adds the _is_cross_id(old) != _is_cross_id(new) rule with a _is_cross_id() helper (voxel_world.gd:1969-1970); ordering is correct (after the attenuation-equality guard, so torch cases still route through the wider light rule; cross→cross swaps like seagrass→kelp stay excluded). BLOCK_SEAGRASS (53) is FLAG_CUTOUT | FLAG_CROSS (block_registry.gd:142), and a verifier case was added (tools/light_invalidation_verify.gd:44-45).

✅ [FIXED] Previous issue: creating a world was committed to disk at menu time. ui/main_menu.gd:253-261 now only applies the config, clears any stale active world, and starts the scene; Main._prepare_world_storage() (main.gd:334-335) performs the durable creation after the scene loads, with the in-memory fallback covering creation failure. The load path (_on_load_world, main_menu.gd:264-273) is unchanged and still activates before scene change.

✅ [FIXED] Previous issue: no-op ternary in decompress_chunk_data. voxel_world.gd:192 is now a plain return PackedByteArray(); no behavior change.

✅ [FIXED] Previous issue: dead latest_saved_world() API. Removed from autoload/game_config.gd; grep confirms zero remaining references.

✅ [FIXED] Previous issue: collision-wait freeze stopped targeting. _update_target() moved above the early return (player.gd:172 precedes the guard at 177-179); it is world-null-safe (player.gd:269) and uses grid-traversal raycasting over chunk data, not collision geometry, so it is correct to run while collision is pending. _update_footsteps() deliberately remains behind the guard — reasonable, since footsteps are distance-driven and velocity is zeroed during the wait.

No new issues introduced by the fix commit were identified. One residual observation (not a defect): when open_world() fails for a corrupt existing world, _prepare_world_storage() still silently creates a fresh world under the same config — this matches the remedy the prior review itself proposed, and the original save remains listed for manual recovery, so I do not re-raise it.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	Fallback logic stays inside _prepare_world_storage/_flush_world_save; VoxelWorld owns null-store semantics; no concern bleed
Open/Closed	8	Version-gated catalogs unchanged; the boundary predicate extended via a new helper without modifying legacy paths
Liskov Substitution	7	Mostly procedural; WorldStorage contract (Dictionary metadata, Error returns) used consistently
Interface Segregation	7	Player↔Main still signals-only; set_edit_store(null) gives VoxelWorld a clean persistence seam
Dependency Inversion	6	Main still depends on the WorldStorage concrete class; acceptable at GDScript scale
Average	7.2

🎯 Final Assessment

Overall Confidence Score: 88%

Confidence Breakdown:

• Code Quality: 90% (fixes are minimal, correctly typed, and each guarded path verified; dead code removed)
• Completeness: 90% (all six review findings addressed, including verifier coverage for the new predicate rule)
• Risk Level: 85% (in-memory fallback eliminates the silent data-loss path; large blast radius otherwise unchanged from the reviewed baseline)
• Verification: 85% (the fix commit touches code covered by light_invalidation_verify.gd, ui_flow_verify.gd, player_target_verify.tscn, world_storage_verify.gd, and chunk_data_compression_verify.gd; the author reports these passing locally — the engine binary is unavailable on this runner so they were not re-executed here, which remains residual risk, not a blocker)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• No unresolved worker-thread, determinism, or data-loss risk

Verdict:

MERGE — all previously reported issues are fixed and verified in the full files, and the fix commit introduces no new problems.

Machine Readable Verdict

{
  "reviewed_sha": "db4ba71bfdee59b7139ef8ec4612ffe0036b02ef",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 88,
  "recommendation": "MERGE"
}

opencode session  |  github run