refactor: split worldgen modules

[MichaelFisher1997]

Summary

• Split worldgen into descriptor-backed modules for API, overworld, flat, and test generators.
• Keep world-worldgen as the facade/registry while preserving existing re-export access patterns.
• Add stable generator IDs/aliases for auto-world resolution and save metadata migration.

Verification

• nix develop --command zig fmt src/ modules/ build.zig
• nix develop --command zig build test

[github-actions]

📋 Summary

No linked issues found in the PR description. This is a structural refactor that splits the monolithic world-worldgen module into descriptor-backed sub-modules (worldgen-api, worldgen-overworld, worldgen-flat, worldgen-test), preserves backward-compatible re-exports, and adds stable generator IDs/aliases for auto-world resolution and save metadata migration. The build and tests pass successfully.

📌 Review Metadata

• Reviewed Commit SHA: ab0cbf2f742627ac9c969aa30bd4f977494f4720
• Reviewed PR: refactor: split worldgen modules #682

This PR cleanly decouples generator implementations from the registry/facade, introduces a proper GeneratorDescriptor API, and makes the auto-world CLI argument resolution dynamic rather than hardcoded. The module wiring in build.zig is correct, and the re-export pattern in world-worldgen/src/root.zig preserves existing consumer access.

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified.

⚠️ High Priority Issues (Should Fix)

[HIGH] modules/worldgen-flat/src/root.zig:32 and modules/worldgen-test/src/root.zig:39 - Missing lighting computation in flat and shadow-test generators
Confidence: High
Description: During the refactor, LightingComputer.computeSkylight() was removed from FlatWorldGenerator.generate() and both computeSkylight() and computeBlockLight() were removed from ShadowTestWorldGenerator.generate(). The overworld generator still performs these calls at modules/worldgen-overworld/src/overworld_generator.zig:221-227, confirming they are still required. There is no evidence of external lighting computation in world-runtime or elsewhere that would compensate for this removal.
Impact: Flat worlds and shadow-test scenes will generate with no/incorrect light data, causing them to render completely dark or with broken lighting. The shadow-test scene specifically exists for lighting/shadow validation, so this regression defeats its purpose.
Suggested Fix:

• In modules/worldgen-flat/src/root.zig, add const LightingComputer = @import("lighting_computer.zig").LightingComputer; and re-insert LightingComputer.computeSkylight(chunk, self.allocator) catch unreachable; before chunk.generated = true;.
• In modules/worldgen-test/src/root.zig, similarly re-import LightingComputer and re-insert both computeSkylight and computeBlockLight calls before chunk.generated = true;.

💡 Medium Priority Issues (Nice to Fix)

None identified.

ℹ️ Low Priority Suggestions (Optional)

[LOW] build.zig:24-27,280 - Unused world_worldgen_options package
Confidence: High
Description: world_worldgen_options is created and passed to the world_worldgen module, but the module no longer imports it after the refactor (chunk debug options moved to worldgen_overworld_options, shadow test variant moved to worldgen_test_options).
Impact: Dead code in the build script. Harmless but should be cleaned up.
Suggested Fix: Remove the world_worldgen_options creation and world_worldgen.addOptions(...) call from build.zig if confirmed unused by all files in modules/world-worldgen/.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	9	Clean split: API types, registry, and each generator implementation now live in dedicated modules.
Open/Closed	8	New generators can be added by creating a new module and registering its descriptor without modifying existing generator code.
Liskov Substitution	8	All generators implement the same Generator vtable via worldgen-api; consumers are unaffected by the internal refactor.
Interface Segregation	9	worldgen-api exposes only the shared contract; individual modules only import what they need.
Dependency Inversion	9	Registry depends on abstract descriptors, not concrete generator implementations.
Average	8.6

🎯 Final Assessment

Overall Confidence Score: 75%

How to interpret: 61-80%: Minor concerns, mostly ready with some fixes

Confidence Breakdown:

• Code Quality: 85% (clean refactor, good module boundaries)
• Completeness: 70% (missing lighting calls are a real regression)
• Risk Level: 70% (the lighting regression affects flat/shadow-test worlds)
• Test Coverage: 80% (existing tests pass, but no new tests for the descriptor system)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (if applicable)

Verdict:

MERGE WITH FIXES

The refactor is architecturally sound and preserves backward compatibility, but the missing lighting computation in flat and shadow-test generators must be restored before merging.

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

opencode session  |  github run

[github-actions]

📋 Summary

No linked issues found in the PR description. This is a structural refactor splitting the monolithic world-worldgen module into descriptor-backed sub-modules (worldgen-api, worldgen-common, worldgen-overworld, worldgen-flat, worldgen-test), preserving backward-compatible re-exports, and adding stable generator IDs/aliases for dynamic auto-world resolution.

📌 Review Metadata

• Reviewed Commit SHA: e6eb39a6953862714c19c02a3fe74d6a077caf2a
• Reviewed PR: refactor: split worldgen modules #682

This PR cleanly decouples generator implementations from the registry/facade, introduces a proper GeneratorDescriptor API, and makes the auto-world CLI argument resolution dynamic rather than hardcoded. The module wiring in build.zig is correct, and the re-export pattern in world-worldgen/src/root.zig preserves existing consumer access. The follow-up commit e6eb39a addressed the lighting regression identified in the previous automated review.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved.

⚠️ High Priority Issues (Should Fix)

✅ [FIXED] Previous issue: Missing lighting computation in flat and shadow-test generators. Both modules/worldgen-flat/src/root.zig:61 and modules/worldgen-test/src/root.zig:58-59 now correctly invoke LightingComputer.computeSkylight() and computeBlockLight() as appropriate.

💡 Medium Priority Issues (Nice to Fix)

None identified.

ℹ️ Low Priority Suggestions (Optional)

✅ [FIXED] Previous issue: Unused world_worldgen_options package. The dead code has been removed from build.zig.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	9	Clean split: API types, shared lighting utilities, registry, and each generator implementation live in dedicated modules.
Open/Closed	8	New generators can be added by creating a new module and registering its descriptor without modifying existing generator code.
Liskov Substitution	8	All generators implement the same Generator vtable via worldgen-api; consumers are unaffected by the internal refactor.
Interface Segregation	9	worldgen-api exposes only the shared contract; individual modules only import what they need.
Dependency Inversion	9	Registry depends on abstract descriptors, not concrete generator implementations.
Average	8.6

🎯 Final Assessment

Overall Confidence Score: 90%

How to interpret: 81-100%: High confidence, ready to merge or with trivial fixes

Confidence Breakdown:

• Code Quality: 90% (clean refactor, good module boundaries, lighting regression fixed)
• Completeness: 90% (descriptor system fully implemented, auto-world resolution dynamic, backward compatibility preserved)
• Risk Level: 85% (structural refactor touching 58 files, but well-contained to worldgen and verified by tests)
• Test Coverage: 90% (existing tests pass, no new descriptor-specific tests but the refactor preserves all existing behavior)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (if applicable)

Verdict:

MERGE

The refactor is architecturally sound, all previously identified issues have been resolved, backward compatibility is preserved, and the full test suite passes.

{
  "reviewed_sha": "e6eb39a6953862714c19c02a3fe74d6a077caf2a",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 90,
  "recommendation": "MERGE"
}

opencode session  |  github run