Skip to content
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

feat: HeightMap mesh builder #170

Merged
merged 8 commits into from
Jan 4, 2025
Merged

feat: HeightMap mesh builder #170

merged 8 commits into from
Jan 4, 2025

Conversation

ManevilleF
Copy link
Owner

@ManevilleF ManevilleF commented Apr 18, 2024

Closes #168

Adds a new HeightMapMeshBuilder with its associated heightmap_builder example

heightmap_builder

@ManevilleF ManevilleF added this to the 0.20.0 milestone Dec 20, 2024
@ManevilleF ManevilleF force-pushed the feat/heightmap branch 3 times, most recently from c899e9e to 01c40c7 Compare January 1, 2025 15:27
@ManevilleF ManevilleF changed the title Custom side management in column mesh builder feat: HeightMap mesh builder Jan 1, 2025
@ManevilleF ManevilleF added enhancement New feature or request breaking-change A breaking change that requires a new major version labels Jan 1, 2025
@ManevilleF ManevilleF marked this pull request as ready for review January 1, 2025 16:45
@rsrasmu2
Copy link

rsrasmu2 commented Jan 2, 2025

This is excellent! I'm unable to try it on my game as a whole until I'm fully updated to 0.15, but I was able to test it on a subset of my terrain generation and it's working quite well! I have a few thoughts and questions, but none of them are blocking.

  1. HeightMapMeshBuilder uses a std::HashMap while Bevy seems to prefer hashbrown::HashMap. It may be worth switching over for easier integration with Bevy.
  2. It would be nice if HeightMapMeshBuilder had a constructor that accepted a HexagonalMap<f32>.
  3. It would also be helpful if HeightMapMeshBuilder allowed you to set the UVs and Options for individual tops and sides, rather than having a singular FaceOptions and UVOptions apply to all hexes in the heighmap.

Thanks for working on this feature, it's going to simplify a good chunk of my codebase!

@ManevilleF
Copy link
Owner Author

1. `HeightMapMeshBuilder` uses a `std::HashMap` while Bevy seems to prefer `hashbrown::HashMap`. It may be worth switching over for easier integration with Bevy.

I'll see what I can do to improve this

2. It would be nice if `HeightMapMeshBuilder` had a constructor that accepted a `HexagonalMap<f32>`.

I guess I could accept a Into<HashMap> or force a dense map like HexagonalMap, and not allow any holes but seems restrictive

3. It would also be helpful if `HeightMapMeshBuilder` allowed you to set the UVs and Options for individual tops and sides, rather than having a singular `FaceOptions` and `UVOptions` apply to all hexes in the heighmap.

Could you give me an example on how that would look like API wise ? A side could be identified by a coordinate and a direction

@rsrasmu2
Copy link

rsrasmu2 commented Jan 2, 2025

I guess I could accept a Into<HashMap> or force a dense map like HexagonalMap, and not allow any holes but seems restrictive

Yeah, no need to do something that would not allow for holes. I just use Hexx's storage types all over my project and it would remove a tiny bit of boilerplate if I could use them with HeightMapMeshBuilder directly.

Could you give me an example on how that would look like API wise ? A side could be identified by a coordinate and a direction

Perhaps mirroring ColumnMeshBuilder with something like

pub const fn with_hex_cap_options(mut self, options: HashMap<Hex, Option<FaceOptions>>) -> Self
pub const fn with_hex_side_options(mut self, options: HashMap<Hex, Option<FaceOptions>>) -> Self
pub const fn with_hex_multi_sides_options(mut self, options: HashMap<Hex, [Option<FaceOptions>;6]>) -> Self

Any hexes that have not had their FaceOptions explicitly set by the above functions use HeightMapMeshBuilder's top_face_options and side_options fields.

@ManevilleF
Copy link
Owner Author

@rsrasmu2 Investigating a bit and I noticed that the storages need to be upgraded to match hashmap behavior, which is a bit more work than I intend for this PR.
I also need to think about the API for the side management, which is is complex due to the fact that we only generate 1 side per pair of neighboring hexagons.

I'm going to merge the PR as it is standing, and we can iterate on those suggestions before the release.

Could you convert your comments/suggestions to an issue ?

@ManevilleF ManevilleF merged commit f7d79f9 into main Jan 4, 2025
9 checks passed
@ManevilleF ManevilleF deleted the feat/heightmap branch January 4, 2025 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change A breaking change that requires a new major version enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support heightmap mesh generation
2 participants