Conversation
…ule-stabilization Feature/multi module stabilization
…ule-stabilization chore: remove obsolete release.yml and update Cargo.lock
…ule-stabilization fix(workflows): update token usage for GitHub Actions
There was a problem hiding this comment.
Pull request overview
This PR merges development work that adds a nutrient system to the 3D voxel world simulation, enhances the biology module with population merging and nutrient-based growth limits, and introduces CI/CD workflows for automated releases. The changes bump the version from 0.1.0 to 0.2.0.
Key Changes:
- Added a
nutrientsfield to theVoxelstruct with default values for different materials (soil: 10.0, water: 5.0) - Implemented nutrient consumption and carrying capacity mechanics in the biology simulation
- Added population merging logic to consolidate populations at the same location
- Introduced GitHub Actions workflows for automated stable and beta releases
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
src/world3d.rs |
Added nutrients field to Voxel struct and updated all constructor methods to initialize nutrients with material-specific defaults |
src/biology.rs |
Implemented population merging, nutrient-based carrying capacity, nutrient consumption mechanics, and increased minimum migration threshold from 0 to 10 |
Cargo.toml |
Version bump from 0.1.0 to 0.2.0 |
Cargo.lock |
Updated version to match Cargo.toml |
CHANGELOG.md |
Added changelog entries documenting the new features and improvements in version 0.2.0 |
.github/workflows/release-stable.yml |
New workflow for automatic stable releases on pushes to main branch |
.github/workflows/release-beta.yml |
New workflow for automatic beta releases on pushes to dev branch |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub material: VoxelMaterial, | ||
| pub temperature: f32, | ||
| pub density: f32, | ||
| pub nutrients: f32, // Ajout des nutriments |
There was a problem hiding this comment.
These comments are in French, but the codebase uses English for all other documentation and comments (e.g., in main.rs and existing world3d.rs comments). For consistency and broader accessibility, please translate these to English. For example: "Ajout des nutriments" → "Addition of nutrients".
| Self::new(VoxelMaterial::Soil, 18.0, 1.2, 10.0) // Ajout de nutriments par défaut pour le sol | ||
| } | ||
|
|
||
| pub fn water() -> Self { | ||
| Self::new(VoxelMaterial::Water, 10.0, 1.0) | ||
| Self::new(VoxelMaterial::Water, 10.0, 1.0, 5.0) // Ajout de nutriments par défaut pour l'eau |
There was a problem hiding this comment.
These comments are in French, but the codebase uses English for all other documentation and comments. For consistency and broader accessibility, please translate these to English. For example: "Ajout de nutriments par défaut pour le sol" → "Default nutrients for soil".
| // Fusionner les populations proches sur le même voxel | ||
| let mut population_map = std::collections::HashMap::new(); | ||
|
|
||
| // Ajouter les populations existantes au map | ||
| for pop in populations.iter() { | ||
| let key = (pop.x, pop.y, pop.z, pop.species_id); | ||
| population_map | ||
| .entry(key) | ||
| .and_modify(|existing_size| *existing_size += pop.size) | ||
| .or_insert(pop.size); | ||
| } | ||
|
|
||
| // Ajouter les nouvelles populations au map | ||
| for pop in new_populations.iter() { | ||
| let key = (pop.x, pop.y, pop.z, pop.species_id); | ||
| population_map | ||
| .entry(key) | ||
| .and_modify(|existing_size| *existing_size += pop.size) | ||
| .or_insert(pop.size); | ||
| } | ||
|
|
||
| // Reconstruire la liste des populations | ||
| populations.clear(); | ||
| for ((x, y, z, species_id), size) in population_map { | ||
| populations.push(Population::new(species_id, x, y, z, size)); | ||
| } |
There was a problem hiding this comment.
The population merging logic at the beginning of the function clears the original populations vector (line 77) and rebuilds it from the HashMap. However, this happens before the retain_mut closure processes populations. This means all populations are being reset before any simulation logic runs, which likely breaks the intended flow. The merging logic should either:
- Run after the retain_mut closure to merge newly created populations with existing ones, or
- Be restructured to preserve the simulation state correctly.
| - name: Create GitHub Beta Release | ||
| uses: actions/create-release@v1 | ||
| with: | ||
| tag_name: ${{ steps.beta.outputs.new_beta_tag }} | ||
| release_name: Beta ${{ steps.beta.outputs.new_beta_tag }} | ||
| body: | | ||
| Beta build from dev. | ||
| draft: false | ||
| prerelease: true | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.PERSONAL_ACCESS_TOKEN }} No newline at end of file |
There was a problem hiding this comment.
The actions/create-release@v1 action is deprecated and no longer maintained. GitHub recommends migrating to the newer softprops/action-gh-release or using the GitHub CLI (gh release create). The deprecated action may stop working in the future.
| && new_z < world.depth as i32 | ||
| { | ||
| // Diviser la population : une partie reste, l'autre se déplace | ||
| let moving_size = pop.size / 2; |
There was a problem hiding this comment.
Changing the population movement threshold from > 0 to > 10 prevents small populations from migrating. This is a significant behavioral change that should be documented in the changelog or explained with a comment, as it affects the simulation dynamics and may prevent small populations from colonizing new areas.
| let moving_size = pop.size / 2; | |
| let moving_size = pop.size / 2; | |
| // Only allow migration if the moving population is greater than 10. | |
| // This threshold prevents small populations from migrating, which may | |
| // affect the ability of small populations to colonize new areas and | |
| // changes the simulation dynamics. Adjust with care. |
| // Ajouter les nouvelles populations au map | ||
| for pop in new_populations.iter() { | ||
| let key = (pop.x, pop.y, pop.z, pop.species_id); | ||
| population_map | ||
| .entry(key) | ||
| .and_modify(|existing_size| *existing_size += pop.size) | ||
| .or_insert(pop.size); | ||
| } |
There was a problem hiding this comment.
The new_populations vector is empty at this point (initialized on line 53), so this loop iterating over it will never execute. This code should be moved to after the retain_mut closure (before line 188) where new_populations is actually populated with new entries. The current placement makes the population merging logic ineffective for newly created populations.
| if new_x >= 0 | ||
| && new_y >= 0 | ||
| && new_z >= 0 | ||
| && new_x < world.width as i32 | ||
| && new_y < world.height as i32 | ||
| && new_z < world.depth as i32 | ||
| { |
There was a problem hiding this comment.
This validation logic duplicates the existing is_valid method available on World3D. Consider using world.is_valid(new_x, new_y, new_z) instead of this manual check for consistency and maintainability.
| if new_x >= 0 | |
| && new_y >= 0 | |
| && new_z >= 0 | |
| && new_x < world.width as i32 | |
| && new_y < world.height as i32 | |
| && new_z < world.depth as i32 | |
| { | |
| if world.is_valid(new_x, new_y, new_z) { |
| uses: actions/create-release@v1 | ||
| with: | ||
| tag_name: ${{ steps.version.outputs.new_tag }} | ||
| release_name: Release ${{ steps.version.outputs.new_tag }} | ||
| body: | | ||
| Stable release ${{ steps.version.outputs.new_tag }}. | ||
| draft: false | ||
| prerelease: false | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.PERSONAL_ACCESS_TOKEN }} No newline at end of file |
There was a problem hiding this comment.
The actions/create-release@v1 action is deprecated and no longer maintained. GitHub recommends migrating to the newer softprops/action-gh-release or using the GitHub CLI (gh release create). The deprecated action may stop working in the future.
| uses: actions/create-release@v1 | |
| with: | |
| tag_name: ${{ steps.version.outputs.new_tag }} | |
| release_name: Release ${{ steps.version.outputs.new_tag }} | |
| body: | | |
| Stable release ${{ steps.version.outputs.new_tag }}. | |
| draft: false | |
| prerelease: false | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.PERSONAL_ACCESS_TOKEN }} | |
| uses: softprops/action-gh-release@v1 | |
| with: | |
| tag_name: ${{ steps.version.outputs.new_tag }} | |
| name: Release ${{ steps.version.outputs.new_tag }} | |
| body: | | |
| Stable release ${{ steps.version.outputs.new_tag }}. | |
| draft: false | |
| prerelease: false | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
| ) { | ||
| let mut rng = rand::thread_rng(); | ||
| let mut new_populations = Vec::new(); | ||
| let mut new_populations: Vec<Population> = Vec::new(); |
There was a problem hiding this comment.
[nitpick] The explicit type annotation : Vec<Population> is redundant here since the type can be inferred from Vec::new(). Rust's type inference makes this unnecessary unless there's ambiguity.
| let mut new_populations: Vec<Population> = Vec::new(); | |
| let mut new_populations = Vec::new(); |
No description provided.