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

Mesh builder #80

Merged
merged 9 commits into from
May 2, 2023
Merged

Mesh builder #80

merged 9 commits into from
May 2, 2023

Conversation

ManevilleF
Copy link
Owner

@ManevilleF ManevilleF commented May 2, 2023

Closes #77

Work done

  • complete rework of mesh procedural generation
  • MeshInfo struct can be merged with other meshes (@alice-i-cecile )
  • Added a ColumnMeshBuilder for column meshes
  • Added a PlaneMeshBuilder for plane meshes
  • Add a mesh_builder example showcasing builder options

@ManevilleF ManevilleF added this to the 0.6.0 milestone May 2, 2023
@ManevilleF ManevilleF changed the title Mesh builder for columns Mesh builder May 2, 2023
@ManevilleF ManevilleF marked this pull request as ready for review May 2, 2023 12:17
@ManevilleF
Copy link
Owner Author

@alice-i-cecile what do you think of the builder API ?

Also, I wonder if I should remove the cheap column I doubt anyone is using it

src/mesh/mod.rs Outdated Show resolved Hide resolved
@ManevilleF ManevilleF added enhancement New feature or request breaking-change A breaking change that requires a new major version code quality Clean up the code usability Make the APIs easier to use labels May 2, 2023
@alice-i-cecile
Copy link
Collaborator

Builder API is nice, although I'd probably make the fields pub. I'd actually keep the cheap hex mesh around: it's useful for creating bounding columns for picking.

@ManevilleF
Copy link
Owner Author

@alice-i-cecile all done, could you check that this PR doesn't break anything in emergence ? Also you might be able to remove the bottom face now and save a few triangles

@alice-i-cecile
Copy link
Collaborator

Yeah, lemme try out a migration :)

@alice-i-cecile
Copy link
Collaborator

Experiments:

  • using deprecated methods still works
  • using new mesh builder works
  • using mesh merging works

image

Mesh merging with its current API is not very useful at all:

  • it's very easy to hit the u16::MAX limit
  • there's no way to specify the translation offsets of the meshes in advance, so they end up collapsed into am overlapping pile
  • I really want to be able to call .reduce with merge_mesh but the type signature is wrong

IMO cut mesh merging from this PR and release.

@ManevilleF
Copy link
Owner Author

The mesh merging method was mostly created for inner usage, it is used by the build method of the new builders and allow to compute different parts of meshes separately.
For your use case, merging all your map together would indeed require a lot of offsets.
I'll investigate looking at the emergence MR to see if I can bring a quick win but otherwise I'll make that method private for now until I start working on chunking

@ManevilleF
Copy link
Owner Author

I added comments to your test attempt, I think you are missing a single param for merging to work as expected.

About the u16 limit, I could consider indices to be u32 but I don't think merging such large meshes make sense, as they should be merged as chunks anyway, otherwise the AABB optimizations in rendering wouldn't apply

@alice-i-cecile
Copy link
Collaborator

Agreed on the chunking point: I wanted to test it out quick and see how it works :)

@ManevilleF ManevilleF merged commit 7103c0b into main May 2, 2023
8 checks passed
@ManevilleF ManevilleF deleted the feat/mesh_builder branch May 2, 2023 17:40
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 code quality Clean up the code enhancement New feature or request usability Make the APIs easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better mesh generation
2 participants