Refactor ResPlan.NET: Remove Building Generator#18
Conversation
- Removed BuildingGenerator.cs and associated tests - Removed Building and BuildingFloor classes from Models.cs - Added GetEntrance() to Plan class - Removed rotation logic from Plan and PlanLoader - Updated PlanRenderer to work directly with Plan objects - Updated Demo to load and render plans individually
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. New to Jules? Learn more at jules.google/docs. |
|
You have successfully removed the BuildingGenerator logic from the C# files, but you failed to update the public contract in your documentation. Any developer (or Agent) reading your README.md will try to instantiate BuildingGenerator and fail immediately. Here is the itemized critique:
You deleted BuildingGenerator.cs and Building models, but your documentation still explicitly sells them as core features. README Section 5: "Multi-Story Building Generation" provides a code sample for new BuildingGenerator(). This code will not compile. README Section 5: References PlanRenderer.RenderFloor(...). This method no longer exists. DOCUMENTATION.md: Still contains a full section on ResPlan.Library.BuildingGenerator and the Building / BuildingFloor types in the Class Diagram. Fix: Burn these sections. The library is now a pure data provider. Do not advertise features you just deleted.
You removed the logic for FrontDoorFacing, but you left the corpse in the method signature: C# // PlanLoader.cs If the library cannot support this constraint anymore (because that's now MapGenerator's job), remove the property from PlanGenerationConstraints entirely. Don't let a user pass a parameter, think it works, and then get a runtime log message (which they might ignore) telling them it does nothing. Action: Delete FrontDoorFacing and GarageFacing from PlanGenerationConstraints.cs completely. Force a compile-time break for consumers so they know the contract has changed.
C# public Coordinate GetEntrance() Deterministic Ordering: You rely on doors.First(). Are you 100% sure the List deserialized from MessagePack/JSON guarantees the exact same order every time? If ResPlan data is noisy and has 2 front doors, MapGenerator needs to pick the same one every time to ensure the "deterministic map generation" promise. Consider sorting by ID or centroid X/Y if there are multiple candidates. Null Safety: This returns null if no door is found. Does MapGenerator handle a null entrance? If a Plan has no entrance, it is effectively useless for your game logic. Suggestion: Change the return type to Coordinate? (nullable) to be explicit, or filter these plans out during the loading phase so MapGenerator never sees a door-less plan.
using NetTopologySuite.Geometries.Utilities; seems unused in the snippet provided. Clean up your imports. Final Verdict Required Actions: Docs: Remove all references to Building, BuildingGenerator, and RenderFloor in README and DOCUMENTATION. Constraints: Delete the FrontDoorFacing property from the PlanGenerationConstraints class. Refinement: Add a deterministic sort to GetEntrance or document that the source data order is trusted. |
Thank you for the detailed feedback. I have addressed the critical documentation failures by removing all references to |
- Updated README.md and DOCUMENTATION.md to remove all references to BuildingGenerator and multi-story features. - Removed FrontDoorFacing and GarageFacing from PlanGenerationConstraints to enforce a cleaner API. - Updated PlanLoader to remove logic related to removed constraints. - Refined Plan.GetEntrance() to use deterministic sorting (Centroid X then Y). - Removed unused using statements in Models.cs.
This PR refactors the ResPlan.NET library to focus on single-level floor plans, removing the multi-story building generation logic. This includes deleting the BuildingGenerator class, updating Models.cs to remove Building and BuildingFloor, and refining the Plan API with a GetEntrance() method. The PlanRenderer and Demo have been updated accordingly. Rotation logic was also removed as it was primarily for building generation.
PR created automatically by Jules for task 10295428911921085118 started by @Xyrces