Add functional blocks and sleeping - #24
Conversation
|
I have now examined the full diff and read the surrounding code in every touched file. No previous automated reviews exist for this PR, so there are no earlier issues to re-check or fixes to acknowledge. 📋 SummaryNo linked issues ("Fixes #X"/"Closes #X") appear in the PR description. The PR implements two ROADMAP items ( Overall this is a well-executed, convention-respecting change: the state-ID encoding is correct (door bit layout verified: facing/open/upper decode consistently), all edit paths route through 📌 Review Metadata
🔴 Critical Issues (Must Fix - Blocks Merge)None identified.
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 8 | State encoding/decoding is centralized in BlockRegistry, shape emission in ChunkMesher, sleep flow in Main; _attempt_sleep mixes guard order with spawn policy slightly |
| Open/Closed | 6 | Adding a shape family touches several match sites (defs, shape_type, canonical_id, placement_variant, mesher _append_custom_block) — explicit but not extensible without editing each |
| Liskov Substitution | 8 | No inheritance introduced; the threat_check callable and place_block default args preserve existing caller contracts (old 2-arg call sites still work) |
| Interface Segregation | 8 | Player→Main still signal-only; interact_check closure extended without new coupling; world exposes narrow helpers (is_threatened, toggle_door) |
| Dependency Inversion | 7 | threat_check: Callable is a clean inversion for the future mob system; mesher/registry remain constructor-injected and immutable for workers |
| Average | 7.4 |
🎯 Final Assessment
Overall Confidence Score: 72%
Confidence Breakdown:
- Code Quality: 85% (clean, commented, convention-following; state math verified correct)
- Completeness: 80% (both roadmap items delivered with recipes/textures/verification; respawn placement is wrong for indoor beds)
- Risk Level: 75% (no worker-thread/determinism/byte-safety regressions found; main risk is the respawn bug and unverified world-level door lifecycle)
- Verification: 65% (broad verifier list in the PR, but the new world-level paths lack coverage and nothing could be re-run on this runner)
Merge Readiness:
- All critical issues resolved (none reported)
- SOLID average score >= 6.0
- Overall confidence >= 60%
- No security concerns
- No unresolved worker-thread, determinism, or data-loss risk (the HIGH issue is gameplay-correctness, not data loss)
Verdict:
MERGE WITH FIXES
The bed-respawn roof bug should be fixed (and ideally covered by a world-level verifier) before merge; everything else is solid, thread-safe, and convention-compliant.
Machine Readable Verdict
{
"reviewed_sha": "baf102619f07d929a2d675b3aaf83079fea5f670",
"critical_issues": 0,
"high_priority_issues": 1,
"medium_priority_issues": 1,
"overall_confidence_score": 72,
"recommendation": "MERGE WITH FIXES"
}|
All verification complete. The fix commit addressed storage but not the effective respawn placement. Compiling the final review. 📋 SummaryNo linked issues ("Fixes #X"/"Closes #X") appear in the PR description. The PR implements two ROADMAP items ( The PR adds byte-safe state IDs 75–108 for stairs/slabs/doors/ladders/signs/beds with shape-specific mesh/collision, canonical-id inventory/drop handling, door toggling, ladder climbing, night sleeping with guards, generated textures, recipes, and a focused verifier. Commit 2 ("Fix indoor bed respawn placement") responds to the previous review: it adds 📌 Review Metadata
🔴 Critical Issues (Must Fix - Blocks Merge)None identified.
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 8 | The fix improves on the previous state: spawn policy now lives in a dedicated find_bed_spawn instead of reusing the surface scanner; respawn() fallback policy remains in pre-existing Player code |
| Open/Closed | 6 | Adding a shape family still touches several match sites (defs, shape_type, canonical_id, placement_variant, mesher) |
| Liskov Substitution | 8 | place_block default args preserve old call sites; find_bed_spawn's Dictionary return is a new narrow contract |
| Interface Segregation | 8 | Player→Main still signal-only; world exposes narrow helpers (find_bed_spawn, toggle_door, is_threatened) |
| Dependency Inversion | 7 | threat_check: Callable remains a clean inversion; mesher/registry stay immutable for workers |
| Average | 7.4 |
🎯 Final Assessment
Overall Confidence Score: 75%
Confidence Breakdown:
- Code Quality: 85% (fix is clean, cell predicate correct, verifier fixture math verified against
_data_indexand chunk lookup paths) - Completeness: 82% (stored respawn and its verification fixed; effective respawn placement still wrong for indoor beds)
- Risk Level: 78% (no worker-thread, determinism, byte-safety, or data-loss risk found; remaining bug is gameplay-correctness)
- Verification: 68% (new indoor-bed verifier added but cannot be run on this runner; door lifecycle still uncovered —
functional_blocks_verify.gdshould be run before merge)
Merge Readiness:
- All critical issues resolved (none reported)
- SOLID average score >= 6.0
- Overall confidence >= 60%
- No security concerns
- No unresolved worker-thread, determinism, or data-loss risk
Verdict:
MERGE WITH FIXES
The fix commit corrects the stored respawn point and adds real coverage for the bed search, but Player.respawn()'s re-scan still discards it and places indoor sleepers on the roof — gate the surface scan behind a cell-validation fallback and this is ready.
Machine Readable Verdict
{
"reviewed_sha": "533122dc4ea78df1f04ed2bc9dccb2e6643ee862",
"critical_issues": 0,
"high_priority_issues": 1,
"medium_priority_issues": 1,
"overall_confidence_score": 75,
"recommendation": "MERGE WITH FIXES"
}|
All checks complete. Commit 3 fixes the HIGH exactly as suggested, with a shared predicate and a verifier assertion; the MEDIUM coverage gap and three LOWs remain. Compiling the final review. 📋 SummaryNo linked issues ("Fixes #X"/"Closes #X") appear in the PR description. The PR implements two ROADMAP items ( Commit 3 ("Preserve validated bed respawns") closes out the previous HIGH issue: 📌 Review Metadata
🔴 Critical Issues (Must Fix - Blocks Merge)✅ All previously reported critical issues have been resolved (none were ever reported).
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 9 | Improved: _is_standable_cell() is now the one shared predicate for bed search, standability, and (via composition) the respawn fallback policy |
| Open/Closed | 6 | Adding a shape family still touches several match sites (defs, shape_type, canonical_id, placement_variant, mesher) |
| Liskov Substitution | 8 | place_block default args preserve old call sites; find_bed_spawn's Dictionary return remains a narrow new contract |
| Interface Segregation | 8 | Player→Main still signal-only; world exposes narrow helpers (is_standable_spawn, find_bed_spawn, toggle_door, is_threatened) |
| Dependency Inversion | 7 | threat_check: Callable remains a clean inversion; mesher/registry stay immutable for workers |
| Average | 7.6 |
🎯 Final Assessment
Overall Confidence Score: 80%
Confidence Breakdown:
- Code Quality: 87% (fix is minimal, correctly gated, and single-sources the cell predicate; no dead code or contract drift)
- Completeness: 88% (both roadmap items delivered; both halves of the respawn bug — stored and effective — are now fixed and pinned by the verifier)
- Risk Level: 82% (no worker-thread, determinism, byte-safety, or data-loss risk found; unload/LOD fallback path verified safe)
- Verification: 68% (verifier extended, but nothing could be re-run on this runner —
functional_blocks_verify.gdshould be run before merge; door lifecycle still uncovered)
Merge Readiness:
- All critical issues resolved (none reported)
- SOLID average score >= 6.0
- Overall confidence >= 60%
- No security concerns
- No unresolved worker-thread, determinism, or data-loss risk
Verdict:
MERGE WITH FIXES
The respawn bug is fully resolved and verified by inspection; extend the verifier to the world-level door/ladder lifecycle (or accept that gap consciously) and this is ready.
Machine Readable Verdict
{
"reviewed_sha": "a269bcfa81f14c427faf3913cdb98772f558363d",
"critical_issues": 0,
"high_priority_issues": 0,
"medium_priority_issues": 1,
"overall_confidence_score": 80,
"recommendation": "MERGE WITH FIXES"
}|
All verification complete. Commit 4 addresses the remaining MEDIUM exactly as previously suggested, and I verified every new assertion against the real 📋 SummaryNo linked issues ("Fixes #X"/"Closes #X") appear in the PR description. The PR implements two ROADMAP items ( The PR adds byte-safe state IDs 75–108 for stairs/slabs/doors/ladders/signs/beds with shape-specific meshes and collision, canonical-id inventory/drop handling, door toggling, ladder climbing, night sleeping with guards, generated textures, recipes, and a focused verifier. Commit 4 ("Cover functional block lifecycles") closes the last open issue: it refactors the verifier's world fixture into a shared 📌 Review Metadata
🔴 Critical Issues (Must Fix - Blocks Merge)✅ All previously reported critical issues have been resolved (none were ever reported).
|
| Principle | Score | Notes |
|---|---|---|
| Single Responsibility | 9 | _is_standable_cell() remains the single shared spawn predicate; _world_fixture() deduplicates verifier setup instead of duplicating fixture wiring |
| Open/Closed | 6 | Adding a shape family still touches several match sites (defs, shape_type, canonical_id, placement_variant, mesher) |
| Liskov Substitution | 8 | place_block default args preserve old call sites; find_bed_spawn's Dictionary return remains a narrow new contract |
| Interface Segregation | 8 | Player→Main still signal-only; world exposes narrow helpers (is_standable_spawn, find_bed_spawn, toggle_door, is_threatened) |
| Dependency Inversion | 7 | threat_check: Callable remains a clean inversion; mesher/registry stay immutable for workers |
| Average | 7.6 |
🎯 Final Assessment
Overall Confidence Score: 85%
Confidence Breakdown:
- Code Quality: 88% (commit 4 is test-only, assertion-rich, and matches the implementation exactly; production code unchanged since the last reviewed fix)
- Completeness: 92% (both roadmap items delivered; both halves of the respawn bug fixed and pinned; the lifecycle coverage gap is closed)
- Risk Level: 84% (no worker-thread, determinism, byte-safety, or data-loss risk; new verifier paths are all headless-safe by construction)
- Verification: 78% (the verifier is now comprehensive across registry, meshes, recipes, sleep, bed spawn, and world lifecycle, but nothing could be executed on this runner — run
redot --headless --path . --script res://tools/functional_blocks_verify.gdand the parse check before merge as routine confirmation)
Merge Readiness:
- All critical issues resolved (none reported)
- 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 blocking and should-fix issues are resolved and verified by inspection; the only remaining items are optional polish nits, and the sole residual risk is that the (now comprehensive) verifier has not been executed on this runner.
Machine Readable Verdict
{
"reviewed_sha": "dfc367108f4070fa8e9bfce84d0e8c7280ba7b99",
"critical_issues": 0,
"high_priority_issues": 0,
"medium_priority_issues": 0,
"overall_confidence_score": 85,
"recommendation": "MERGE"
}



Summary
Verification
redot --editor --headless --path . --quitredot --headless --path . --script res://tools/functional_blocks_verify.gdredot --headless --path . --script res://tools/inventory_crafting_verify.gdredot --headless --path . --script res://tools/inventory_ui_verify.gdredot --headless --path . --script res://tools/audio_verify.gdredot --headless --path . --script res://tools/weather_verify.gdredot --headless --path . --script res://tools/gameplay_integration_verify.gdredot --headless --path . res://tools/player_target_verify.tscnredot --headless --path . res://tools/explosives_verify.tscnredot --headless --path . res://tools/fire_verify.tscnredot --headless --path . --script res://tools/world_storage_verify.gdredot --headless --path . --script res://tools/chunk_data_compression_verify.gd