-
Notifications
You must be signed in to change notification settings - Fork 0
Tree generation #109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tree generation #109
Conversation
Reorganized and renamed map generation classes, replacing MainWorldGenerationStrategy with OverworldGenerationStrategy. Introduced MapGenerationParameters and GenerationProcess for improved extensibility. Moved and refactored biome, points, and structure generation steps under an overworld package. Updated usages and tests to reflect the new structure and naming.
Moved biome decision logic from BiomeGenerationStep to BiomeParameters for better encapsulation. Introduced a new structure generation system, including StructureGenerationStep, StructureGenerationConfig, and TreeGenerationConfig, enabling procedural placement of structures like trees based on biome parameters and noise. Updated world and tile logic to support actor rendering and placement, and refactored noise generator constructors for consistency.
Actors are now rendered by iterating over tiles in relevant chunks instead of directly from a global actor list. This ensures actors are rendered on top of tiles and improves rendering locality. Also adds a tiles() method to Chunk for easier tile access, and removes actor rendering from Tile's render method. Minor cleanup in structure generation logging.
Summary of ChangesHello @Lunkle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the foundational elements for procedural tree generation within the game world, significantly enhancing environmental detail. It achieves this by overhauling the existing world generation architecture, centralizing generation steps, and encapsulating biome determination logic. The changes also include performance improvements for actor rendering and a general cleanup of generation-related class names and packages. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant and well-executed refactoring of the world generation system. The new GenerationProcess with distinct steps for biomes, points of interest, and now structures, greatly improves the modularity and extensibility of the generation logic. The addition of tree generation is a great new feature. I've also noticed and appreciated the performance optimization in actor rendering.
I have a few suggestions to further improve the code:
- A bug fix for actor rendering logic to ensure all nearby actors are rendered.
- A small refactoring to improve consistency in how generation parameters are accessed.
- A design consideration for handling conflicts in structure generation.
- Removal of an unused method.
Overall, this is a high-quality pull request.
| List<Chunk> chunksToRender = asList( | ||
| getChunk(chunkCoord), | ||
| getChunk(chunkCoord.up()), | ||
| getChunk(chunkCoord.down()), | ||
| getChunk(chunkCoord.left()), | ||
| getChunk(chunkCoord.right()), | ||
| getChunk(chunkCoord.down().right()), | ||
| getChunk(chunkCoord.right().right()), | ||
| getChunk(chunkCoord.down().down()) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for selecting chunks to render appears to be flawed. It doesn't select a complete 3x3 grid of chunks around the camera's current chunk, which could lead to actors at the edges of the screen not being rendered. The current implementation includes chunks like chunkCoord.right().right() and chunkCoord.down().down(), which are outside the immediate 3x3 area, while missing corner chunks like up().left().
To ensure all visible actors are rendered correctly, I suggest using a proper 3x3 grid of chunks centered on the camera's position.
List<Chunk> chunksToRender = asList(
getChunk(chunkCoord.up().left()),
getChunk(chunkCoord.up()),
getChunk(chunkCoord.up().right()),
getChunk(chunkCoord.left()),
getChunk(chunkCoord),
getChunk(chunkCoord.right()),
getChunk(chunkCoord.down().left()),
getChunk(chunkCoord.down()),
getChunk(chunkCoord.down().right())
);| public GenerationProcess(Zone zone, MapGenerationStrategy strategy) { | ||
| biome = new BiomeGenerationStep(zone, strategy.seed()); | ||
| points = new PointsGenerationStep(zone, strategy.seed()); | ||
| structure = new StructureGenerationStep(zone, strategy.parameters()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an inconsistency in how generation steps are initialized. BiomeGenerationStep and PointsGenerationStep use strategy.seed(), while StructureGenerationStep uses strategy.parameters(). The seed() method on MapGenerationStrategy appears to be redundant now that MapGenerationParameters exists.
For better consistency and to centralize parameter access, I recommend using strategy.parameters().seed() for all step initializations. This would also allow for the removal of the seed() method from the MapGenerationStrategy interface and its implementations, cleaning up the API.
| public GenerationProcess(Zone zone, MapGenerationStrategy strategy) { | |
| biome = new BiomeGenerationStep(zone, strategy.seed()); | |
| points = new PointsGenerationStep(zone, strategy.seed()); | |
| structure = new StructureGenerationStep(zone, strategy.parameters()); | |
| } | |
| public GenerationProcess(Zone zone, MapGenerationStrategy strategy) { | |
| biome = new BiomeGenerationStep(zone, strategy.parameters().seed()); | |
| points = new PointsGenerationStep(zone, strategy.parameters().seed()); | |
| structure = new StructureGenerationStep(zone, strategy.parameters()); | |
| } |
| for (StructureGenerationConfig params : structureParameters) { | ||
| BiomeParameters biomeParameters = zone.biomeGenerationStep().parametersAt(tile); | ||
| Structure structure = params.placeStructure(tile, biomeParameters); | ||
| structures[chunk.x() * CHUNK_SIZE + tile.x()][chunk.y() * CHUNK_SIZE + tile.y()] = structure; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the generate method, you iterate through structureParameters and assign a structure to a tile. If multiple StructureGenerationConfig instances attempt to place a structure on the same tile, the last one in the list will overwrite the previous ones. This might be the intended behavior, but it could lead to unexpected results as more structure types are added.
You might want to consider adding a check to see if a tile is already occupied by a structure before placing a new one to prevent this.
...ealms/context/game/world/map/generation/overworld/structure/config/TreeGenerationConfig.java
Outdated
Show resolved
Hide resolved
Removed the seed() method from MapGenerationStrategy and updated all generation steps (biome, points, structure) to use strategy.parameters().seed() instead. Adjusted constructors and usages accordingly for improved consistency and extensibility. Added missing no-arg constructors for serialization where needed.
No description provided.