Burn blocks in place with a fire overlay

[MichaelFisher1997]

Summary

Fire no longer swaps a flammable block for a fire block. Burning is now a three-state lifecycle:

1. Normal — the block is untouched.
2. On fire — the block enters a _burning state for FUEL_BURN_TICKS with its texture and collision intact. A new FireOverlay (world/fire_overlay.gd + fire_overlay.gdshader/smoke_overlay.gdshader) clusters animated flame and smoke billboards on it (one MultiMesh per effect, animated in-shader from per-instance phase/progress, refreshed only on the 4 Hz fire tick). While burning it spreads into flammable neighbours one at a time.
3. Burnt out — the block crumbles to air and disappears; that edit goes through _record_edit(), so the settled result survives chunk regeneration.

Spreading is paced so a tree burns gradually (~2 blocks/s): a per-block FIRE_IGNITION_DELAY_TICKS (4) before a new block can spread, FIRE_SPREAD_PER_TICK (1) on every FIRE_SPREAD_PERIOD_TICKS (2), plus a per-cell FIRE_SPREAD_INTERVAL_TICKS cooldown. Burning can chain-detonate adjacent TNT/nukes.

BLOCK_FIRE (70) is now only the standalone flame flint-and-steel leaves on a non-flammable face (e.g. stone); clicking a log/leaf starts that block burning instead of replacing it. BlockRegistry.FLAG_FLAMMABLE still marks logs, leaves, dry plants, and mushrooms as fuel.

Assets

• assets/placeholders/fire/ adds a generated flame sheet and a smoke puff (tools/gen_fire_texture.gd, deterministic), with a README noting provenance.

Tests

• tools/fire_verify.tscn covers flammability flags, burn-in-place, spread pacing, burn-out to persistent air, decay, chain detonation, flint-and-steel routing, persistence reseeding, and the shader/overlay wiring.
• Added to the CI fast-checks shard.

Verified locally (rebased on current main): parse check clean; fire_verify, explosives_verify, player_target_verify, all worldgen_* + stream_full_verify, and the audio/weather/ui/input/photo/frame-pacing verifiers pass.

[github-actions]

📋 Review of PR #20 — Burn blocks in place with a fire overlay

📌 Review Metadata

• Reviewed Commit SHA: 08b295f5964cfaa5aa3e7b7a0373529e9e7b5f33
• Reviewed PR: Burn blocks in place with a fire overlay #20
• Linked issues: None referenced in the PR body (no "Fixes/Closes #" found).
• Previous automated reviews: None — this is the first review, so there are no prior issues to re-verify or fixes to acknowledge.

Summary: This PR replaces the old "fire swaps the block" behavior with a three-state burn lifecycle: flammable blocks burn in place (voxel untouched, collision intact) with a new FireOverlay MultiMesh flame/smoke billboard system animated in-shader and refreshed only on the 4 Hz fire tick, then crumble to a persisted AIR edit. A standalone BLOCK_FIRE (70) cross block remains for flint-and-steel on non-flammable faces, spreading is globally/cooldown paced, and burning cells chain-detonate adjacent TNT/nukes. Implementation quality is high: the sim is main-thread-only, follows the water-tick batching pattern (one version bump + light-ring rebuild per chunk per tick), all voxel changes route through _record_edit(), and the PR ships a dedicated scene-based verifier wired into CI plus AGENTS/ROADMAP updates. The main gap is that the fire simulation ignores water entirely.

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified. Worker-thread safety is clean (all fire state is touched only from _process/Main on the main thread; _seed_fire_edits runs in _commit_chunk which is main-thread), determinism is unaffected (no worldgen/noise changes; overlay placement is hash-derived), persistence flows through the existing _edited_blocks/_edits_by_chunk mirror, and BLOCK_FIRE (70) stays well under the PackedByteArray limit with flag values ≤ 202 < 256.

⚠️ High Priority Issues (Should Fix)

None identified. I specifically verified: queue snapshot/requeue logic cannot double-process or lose cells (voxel_world.gd:1158-1197), _burning[position] cannot be missing when read (_start_burning never erases; carve_sphere doesn't touch fire state), LOD/unloaded chunks are refused on every fire write path (_ignite_fire_cell, _extinguish_fire, _destroy_burnt_block, and spread targets fail is_flammable(AIR)), mid-tick trigger_explosive → carve_sphere → _queue_rebuild interleaves safely with the separate fire arrays, and the fire_layer shader uniform default of -1 disables the animation for all real layers.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] world/voxel_world.gd:1119-1127 and world/voxel_world.gd:1304-1310 - Fire simulation ignores water; burnt blocks never seed water
Confidence: High
Description: The fire paths never consult water. _start_burning() ignites a flammable block even when it is submerged (the player raycast skips water, so a submerged mangrove log is a legal target, and mangrove logs/roots are now FLAG_FLAMMABLE), and _destroy_burnt_block()/_extinguish_fire() write AIR without calling _seed_water(). break_block() (voxel_world.gd:940) and carve_sphere() (voxel_world.gd:1044-1050) both seed water so adjacent fluid refills the hole — the fire lifecycle is the only edit path that doesn't.
Impact: Burning a log at/below a waterline (swamp mangroves, a lake pier the player built) leaves permanent glassy air pockets under water that never refill, persisted forever through _edited_blocks. It also reads wrong that fire ignites and consumes fuel underwater at all.
Suggested Fix: Refuse ignition when the cell is wet, and seed water when a cell is destroyed, e.g.:

func _start_burning(block_position: Vector3i, _changed_chunks: Dictionary) -> bool:
	if not _blocks.is_flammable(get_block_world(block_position)):
		return false
	if _blocks.is_water_id(get_block_world(block_position + Vector3i(0, 1, 0))):
		return false
	...

func _destroy_burnt_block(position: Vector3i, changed_chunks: Dictionary) -> void:
	...
	chunk.data[_data_index(position)] = BlockRegistry.BLOCK_AIR
	_record_edit(position, BlockRegistry.BLOCK_AIR)
	_seed_water(position)
	changed_chunks[_chunk_for_block(position)] = true

Add an underwater-ignition case to tools/fire_verify.gd if adopted.

ℹ️ Low Priority Suggestions (Optional)

[LOW] world/fire_overlay.gd:8 (MAX_CELLS := 384) - Burning-cell cap silently drops flames at the fire front
Confidence: High
Description: refresh() iterates _burning in insertion order and stops at 384 cells. With spread at ~2 blocks/s, a sustained forest fire reaches the cap in ~3 minutes, and the cells dropped are the most recently ignited — the active front the player is watching — while stale early cells keep their billboards.
Impact: Large fires show burning blocks (which still crumble) with no flame/smoke visuals; purely cosmetic.
Suggested Fix: Either raise the cap, or when over budget prefer cells nearest the player/camera (_burning keys sorted by distance to Camera3D position passed into refresh()).

[LOW] world/voxel_world.gd:1063-1065 + game/main.gd:728-730 - Flint and steel on an already-burning block reports "needs a solid face to light"
Confidence: High
Description: Clicking a flammable block that is already in _burning makes ignite_fire() → _start_burning() return false, so use_flint_and_steel() returns {} and Main shows the face-related hint, which is misleading for this case. Relatedly, fire placed on a non-opaque, non-flammable support (glass) passes _ignite_fire_cell but _fire_supported() rejects it, so it extinguishes one tick after "Lit fire".
Impact: Minor UX confusion; no state corruption.
Suggested Fix: Have _start_burning return true (no-op success) when _burning.has(position), or return a distinguishable payload so Main can show "Already burning"; optionally treat any non-air, non-water neighbor as support in _fire_supported.

[LOW] world/voxel_world.gd:1245-1265 - Burning cell destroyed by a chain blast still counts down and re-records its AIR edit
Confidence: Medium
Description: In _update_burning_cell, a neighbor's trigger_explosive() can carve the burning block itself mid-call; the code then continues with int(_burning[position]) - 1 and, if the timer hits 0 in the same call, _destroy_burnt_block() re-sets AIR, re-records the AIR edit, and adds an extra _chunk_edit_version bump/rebuild for a cell carve_sphere already handled.
Impact: Redundant edit + rebuild work only; self-heals on the next tick's flammability check. No data corruption.
Suggested Fix: Re-check liveness after the neighbor loop:

	if not _blocks.is_flammable(get_block_world(position)):
		_burning.erase(position)
		_fire_spread_cooldown.erase(position)
		return
	var ticks := int(_burning[position]) - 1

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	6	FireOverlay, shaders, and the registry flag are cleanly separated, but voxel_world.gd absorbs another ~330 lines of simulation (now 1608 lines) beside water/explosives; a FireSim helper would match the water-tick scale better
Open/Closed	8	New FLAG_FLAMMABLE extends the def-table instead of branching on ids; shader gains a disabled-by-default fire_layer uniform; existing blocks unaffected
Liskov Substitution	8	FireOverlay extends Node3D with no contract violations; registry helpers are pure additions
Interface Segregation	6	use_flint_and_steel reuses the stringly-typed Dictionary payload of trigger_explosive — consistent with the codebase, though "ignited" is only meaningful on one branch
Dependency Inversion	7	Main stays decoupled via the existing item_used signal and the world-facing API; Player.target_normal reuse keeps the signal contract intact
Average	7.0

🎯 Final Assessment

Overall Confidence Score: 78%

Confidence Breakdown:

• Code Quality: 88% — idiomatic, well-commented, mirrors the water-tick batching and edit-persistence invariants precisely
• Completeness: 85% — roadmap item genuinely fulfilled (spread, decay, ignition, chain detonation, persistence reseeding, assets, docs, CI)
• Risk Level: 82% — no worker-thread, determinism, or data-loss exposure found; the water-interaction gap is the main correctness residue
• Verification: 65% — tools/fire_verify.tscn coverage is strong and CI-wired, but it could not be run on this runner (engine unavailable per instructions); residual risk also noted for the visual overlay (needs a display run, e.g. redot -path . res://tools/fire_verify.tscn plus an in-game check)

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 WITH FIXES — Solid, convention-following implementation with no blocking risks; address the water/fire interplay gap (underwater ignition + missing _seed_water on burn-out) either in this PR or an immediate follow-up, and confirm by running tools/fire_verify.tscn and the parse check once the engine is available.

{
  "reviewed_sha": "08b295f5964cfaa5aa3e7b7a0373529e9e7b5f33",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 1,
  "overall_confidence_score": 78,
  "recommendation": "MERGE WITH FIXES"
}

opencode session  |  github run

[github-actions]

All previous findings are addressed, and the fixes check out against the full-file invariants. The local checkout is exactly the PR head 4704e2a (clean tree), so the files read are the reviewed code. Composing the review.

📋 Summary

No linked issues in the PR body (no "Fixes/Closes #" found).

Commit 4704e2a ("Address fire review feedback") responds to every finding from the first automated review: fire now refuses flooded cells and reports why, burn-out/extinguish seed adjacent water exactly like break_block()/carve_sphere(), the overlay keeps the camera-nearest cells when over its cap, flint-and-steel distinguishes "wet"/"already burning" payloads, _fire_supported() accepts any non-air/non-water neighbor (glass support, matching Minecraft), and _update_burning_cell() re-checks liveness after a mid-tick chain blast. Each fix carries new cases in tools/fire_verify.gd (_check_submerged_and_support). I traced the new code through the full files and found the queue/persistence/water-seeding invariants intact (_seed_water queues neighbors unconditionally so the new test's expectation is valid; _fire_tick never drains _water_queued).

📌 Review Metadata

• Reviewed Commit SHA: 4704e2a2335e50f2e478885557a5ab65fe5aa7cf
• Reviewed PR: Burn blocks in place with a fire overlay #20
• Linked issues: None.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved (none were reported).

None identified. Re-verified on the new commit: fire state remains main-thread-only (_fire_tick from _process, use_flint_and_steel via the Player signal), _seed_fire_edits still runs in _commit_chunk (main thread), persistence still flows through _record_edit()/_edits_by_chunk, and FLAG_FLAMMABLE (128) keeps max flag combinations at 234 < 256.

⚠️ High Priority Issues (Should Fix)

✅ All previously reported high priority issues have been resolved (none were reported).

None identified. Specifically re-verified the new code paths: the wet/burning early-returns in use_flint_and_steel (voxel_world.gd:1063-1067) cannot mask the TNT branch (checked first at line 1059); _extinguish_fire/_destroy_burnt_block call _seed_water after _record_edit but before the chunk rebuild flush, matching break_block() ordering; and refresh()'s viewport query (fire_overlay.gd:79) only runs on the main thread (fire tick / ignite paths).

💡 Medium Priority Issues (Nice to Fix)

✅ [FIXED] Previous issue: fire simulation ignored water — submerged blocks could ignite and burnt blocks left permanent air pockets under water.

• _start_burning() refuses _is_submerged() cells (voxel_world.gd:1132-1133), use_flint_and_steel() returns {"blocked": "wet"} (voxel_world.gd:1064-1065), and both _extinguish_fire() (voxel_world.gd:1318) and _destroy_burnt_block() (voxel_world.gd:1331) call _seed_water() so adjacent fluid refills the hole. Covered by _check_submerged_and_support in tools/fire_verify.gd:139-175.

None identified.

ℹ️ Low Priority Suggestions (Optional)

✅ [FIXED] Previous issue: overlay MAX_CELLS cap dropped the newest (active-front) cells — refresh() now sorts by distance to the camera and keeps the nearest 384 (world/fire_overlay.gd:74-84), with zero cost under the cap.

✅ [FIXED] Previous issue: flint-and-steel on an already-burning block showed the wrong face hint, and fire on glass died instantly — {"blocked": "burning"} payload surfaces as "Already burning" (game/main.gd:731-733) and _fire_supported() now accepts any non-air, non-water neighbor (voxel_world.gd:1299-1305). Adjacent fire cells still die naturally since fueled requires a flammable neighbor.

✅ [FIXED] Previous issue: a chain-blasted burning cell re-recorded its AIR edit — _update_burning_cell() now re-checks flammability after the neighbor loop and bails (voxel_world.gd:1273-1278).

[LOW] world/voxel_world.gd:1079-1080 - _is_submerged() only samples the cell directly above
Confidence: Medium
Description: This implements exactly the previously suggested remedy, and the reported defect (underwater ignition plus permanent air pockets) is fixed. The residual edge: a flammable block with water only on a lateral face (a log at the exact waterline) can still ignite and burn, since only position + Vector3i(0, 1, 0) is checked.
Impact: Cosmetic oddity only — when the block crumbles, the new _seed_water() call refills the hole, so no persistent artifact remains.
Suggested Fix:

func _is_submerged(block_position: Vector3i) -> bool:
	for offset in FIRE_OFFSETS:
		if _blocks.is_water_id(get_block_world(block_position + offset)):
			return true
	return false

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	6	Overlay/shaders/registry stay separated, but voxel_world.gd grows to 1630 lines with fire beside water and explosives; a FireSim helper remains the natural next refactor
Open/Closed	8	Fix extends behavior through existing seams (flag table, edit/water-seed pipeline, payload dictionary) without branching on block ids
Lishov Substitution	8	FireOverlay extends Node3D unchanged; new _is_submerged is a pure query
Interface Segregation	7	The blocked string disambiguates the flint-and-steel payload without breaking the existing Dictionary contract Main already consumes
Dependency Inversion	7	Main stays decoupled via the item_used signal; refresh() now takes a camera dependency implicitly via the viewport, acceptable for a presentation node
Average	7.2

🎯 Final Assessment

Overall Confidence Score: 86%

Confidence Breakdown:

• Code Quality: 88% — fixes are minimal, idiomatic, and mirror the break_block()/carve_sphere() conventions they align with
• Completeness: 92% — all four review findings addressed with dedicated test coverage; only the lateral-water nuance remains (cosmetic)
• Risk Level: 85% — no worker-thread, determinism, or data-loss exposure; new water seeding routes through the existing _water_tick queue
• Verification: 78% — tools/fire_verify.tscn (incl. the new _check_submerged_and_support) could not be run on this runner (engine unavailable); the author reports local passes, and I traced the test's expectations through _seed_water/_queue_water manually — run the parse check plus redot --headless --path . res://tools/fire_verify.tscn where the engine is available to confirm

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 with test coverage, and no new blocking concerns were introduced; the one remaining LOW (lateral-water ignition) is cosmetic and self-healing.

{
  "reviewed_sha": "4704e2a2335e50f2e478885557a5ab65fe5aa7cf",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 86,
  "recommendation": "MERGE"
}

opencode session  |  github run

[github-actions]

All previous findings verified fixed on the new head 1600011; the only change since the last review is the _is_submerged broadening plus its test. Composing the review.

📋 Summary

No linked issues in the PR body (no "Fixes/Closes #" found).

Commit 1600011 ("Treat any water-touching block as impossible to ignite") resolves the last remaining finding from the second review: _is_submerged() (world/voxel_world.gd:1080-1084) now checks all six FIRE_OFFSETS neighbors instead of only the cell above, so a log at the exact waterline with water on a lateral face is refused ignition, both via flint-and-steel ({"blocked": "wet"}) and via fire spread. The fix carries a dedicated shoreline-log case in tools/fire_verify.gd:149-155, whose block placement I verified sits inside chunk zero and matches the implementation's lookup path. The rest of the PR is unchanged from 4704e2a, which the prior review already fully cleared; I re-traced the fire lifecycle on this head and the queue-snapshot/requeue, edit-persistence, water-seeding, and main-thread-only invariants all still hold.

📌 Review Metadata

• Reviewed Commit SHA: 16000117f465a44da551f6fc9a601de524f256ad
• Reviewed PR: Burn blocks in place with a fire overlay #20
• Linked issues: None.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved (none were reported).

None identified. Re-verified on this head: fire state remains main-thread-only (_fire_tick from _process, use_flint_and_steel via item_used in game/main.gd:727), _seed_fire_edits still runs from _commit_chunk, all voxel changes still route through _record_edit()/_edits_by_chunk, and the broadened _is_submerged is only called on ignition paths (a handful of get_block_world calls per tick at most, never per-frame).

⚠️ High Priority Issues (Should Fix)

✅ All previously reported high priority issues have been resolved (none were reported).

None identified. The _burning[position] read at voxel_world.gd:1283 stays protected by the post-blast liveness re-check at 1279; every erase path either fails to re-queue or erases-and-returns before the timer logic, so a snapshot cell can never be missing from _burning when processed.

💡 Medium Priority Issues (Nice to Fix)

✅ [FIXED] Previous issue: fire simulation ignored water — submerged blocks could ignite and burnt blocks left permanent air pockets under water (fixed in 4704e2a, re-verified intact on this head: _start_burning refuses _is_submerged() cells, use_flint_and_steel returns {"blocked": "wet"}, and _extinguish_fire/_destroy_burnt_block both call _seed_water()).

None identified.

ℹ️ Low Priority Suggestions (Optional)

✅ [FIXED] Previous issue: _is_submerged() only sampled the cell directly above, allowing lateral-waterline ignition — now all six faces are checked via FIRE_OFFSETS (world/voxel_world.gd:1080-1084), covered by the new shoreline case in tools/fire_verify.gd:149-155.

✅ [FIXED] Previous issues: overlay MAX_CELLS cap dropping the active front (camera-nearest sort), already-burning/wet flint-and-steel payloads, and the chain-blasted burning cell re-recording its AIR edit — all verified still present and unchanged on this head.

[LOW] world/voxel_world.gd:1250-1290 - An already-burning block is not doused when water arrives
Confidence: Medium
Description: The stricter _is_submerged() creates a small asymmetry: it gates ignition, but _update_burning_cell() never re-checks it. A log that ignites dry and is later encroached on by flowing water (water cannot replace the solid log, so the block survives) keeps burning to completion. This is the flip side of the new rule and only matters for the short window while the player diverts water onto a fire.
Impact: Cosmetic; the block is intact the whole time and burn-out calls _seed_water(), so no persistent artifact results. Spread out of the wet cell into other submerged fuel is already blocked by _start_burning.
Suggested Fix: Douse instead of burn when flooded:

func _update_burning_cell(position: Vector3i, changed_chunks: Dictionary) -> void:
	if _loaded_chunk_for(position) == null:
		_burning.erase(position)
		_fire_spread_cooldown.erase(position)
		return
	if not _blocks.is_flammable(get_block_world(position)) or _is_submerged(position):
		_burning.erase(position)
		_fire_spread_cooldown.erase(position)
		return

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	6	Overlay/shaders/registry stay separated, but voxel_world.gd is now 1634 lines with fire beside water and explosives; a FireSim helper remains the natural next refactor
Open/Closed	8	_is_submerged broadens behavior through the existing offset table without branching on block ids; no existing semantics broken
Liskov Substitution	8	FireOverlay extends Node3D unchanged; _is_submerged remains a pure query
Interface Segregation	7	The blocked string disambiguates the flint-and-steel Dictionary payload without breaking the contract Main consumes
Dependency Inversion	7	Main stays decoupled via item_used; the overlay takes its camera dependency implicitly via the viewport
Average	7.2

🎯 Final Assessment

Overall Confidence Score: 88%

Confidence Breakdown:

• Code Quality: 88% — the incremental fix is minimal, idiomatic, comment-updated, and matches the previously suggested remedy exactly
• Completeness: 95% — every finding across both prior reviews is now addressed with dedicated verifier coverage; only the wet-while-burning nuance remains (cosmetic, self-healing)
• Risk Level: 87% — no worker-thread, determinism, or data-loss exposure; the broadened check adds at most six lookups per ignition attempt, none in per-frame paths
• Verification: 78% — tools/fire_verify.tscn (including the new shoreline case) could not be run on this runner (engine unavailable); expectations were traced manually through _is_submerged/use_flint_and_steel and are consistent — run the parse check plus redot --headless --path . res://tools/fire_verify.tscn where the engine is available to confirm

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 — The final remaining finding is fixed with test coverage and no new issues were introduced; the one residual LOW (water not dousing an already-burning block) is cosmetic and a natural follow-up.

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

opencode session  |  github run