Added code that allows for easier rendering of meshes #108
Conversation
Summary of ChangesHello @Gip-Gip, 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 streamlines the process of preparing triangulated meshes for rendering by introducing a new 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable utility for preparing meshes for rendering by converting them to a more GPU-friendly format. The implementation correctly handles vertex deduplication and data type conversion. My review focuses on improving the performance and readability of the new build_graphics_mesh function. I've suggested more accurate capacity allocation for vectors to reduce reallocations and refactoring parts of the vertex processing loop to be more idiomatic and efficient.
src/mesh/mod.rs
Outdated
|
|
||
| let triangle_count = triangles.len(); | ||
|
|
||
| let mut indices: Vec<u32> = Vec::with_capacity(triangle_count); |
There was a problem hiding this comment.
The capacity for the indices vector is initialized to triangle_count, but it will eventually hold triangle_count * 3 elements. This will likely cause multiple reallocations for large meshes, impacting performance. Initializing it with the final capacity is more efficient.
| let mut indices: Vec<u32> = Vec::with_capacity(triangle_count); | |
| let mut indices: Vec<u32> = Vec::with_capacity(triangle_count * 3); |
src/mesh/mod.rs
Outdated
| let mut vertices: Vec<GraphicsMeshVertex> = Vec::with_capacity(triangle_count); | ||
| const VERT_DIM_SIZE: usize = std::mem::size_of::<[f32; 3]>(); | ||
| let mut vertices_hash: HashMap<([u8; VERT_DIM_SIZE], [u8; VERT_DIM_SIZE]), u32> = | ||
| HashMap::with_capacity(triangle_count); |
There was a problem hiding this comment.
The capacity for vertices and vertices_hash is set to triangle_count. For many meshes, the number of unique vertices differs significantly from the number of triangles. For example, in a closed manifold mesh, the number of vertices is typically about half the number of triangles. The PR description itself implies a vertex-to-triangle ratio of about 1.3. Using triangle_count as capacity may lead to unnecessary reallocations. A better heuristic, like triangle_count * 3 / 2, could provide a more reasonable starting capacity.
| let mut vertices: Vec<GraphicsMeshVertex> = Vec::with_capacity(triangle_count); | |
| const VERT_DIM_SIZE: usize = std::mem::size_of::<[f32; 3]>(); | |
| let mut vertices_hash: HashMap<([u8; VERT_DIM_SIZE], [u8; VERT_DIM_SIZE]), u32> = | |
| HashMap::with_capacity(triangle_count); | |
| let mut vertices: Vec<GraphicsMeshVertex> = Vec::with_capacity(triangle_count * 3 / 2); | |
| const VERT_DIM_SIZE: usize = std::mem::size_of::<[f32; 3]>(); | |
| let mut vertices_hash: HashMap<([u8; VERT_DIM_SIZE], [u8; VERT_DIM_SIZE]), u32> = | |
| HashMap::with_capacity(triangle_count * 3 / 2); |
src/mesh/mod.rs
Outdated
| let pos_x = vertex.pos.x as f32; | ||
| let pos_y = vertex.pos.y as f32; | ||
| let pos_z = vertex.pos.z as f32; | ||
|
|
||
| let norm_x = vertex.normal.x as f32; | ||
| let norm_y = vertex.normal.y as f32; | ||
| let norm_z = vertex.normal.z as f32; | ||
|
|
||
| let pos_xyz = [pos_x, pos_y, pos_z]; | ||
| let norm_xyz = [norm_x, norm_y, norm_z]; | ||
|
|
||
| let pos_xyz_bytes: [u8; std::mem::size_of::<[f32; 3]>()] = cast(pos_xyz); | ||
| let norm_xyz_bytes: [u8; std::mem::size_of::<[f32; 3]>()] = cast(norm_xyz); |
There was a problem hiding this comment.
This block can be simplified. nalgebra provides a cast method to convert between numeric types for points and vectors, which is cleaner than casting each component individually. Additionally, the VERT_DIM_SIZE constant defined on line 263 should be used here for consistency instead of calling std::mem::size_of again.
| let pos_x = vertex.pos.x as f32; | |
| let pos_y = vertex.pos.y as f32; | |
| let pos_z = vertex.pos.z as f32; | |
| let norm_x = vertex.normal.x as f32; | |
| let norm_y = vertex.normal.y as f32; | |
| let norm_z = vertex.normal.z as f32; | |
| let pos_xyz = [pos_x, pos_y, pos_z]; | |
| let norm_xyz = [norm_x, norm_y, norm_z]; | |
| let pos_xyz_bytes: [u8; std::mem::size_of::<[f32; 3]>()] = cast(pos_xyz); | |
| let norm_xyz_bytes: [u8; std::mem::size_of::<[f32; 3]>()] = cast(norm_xyz); | |
| let pos_xyz: [f32; 3] = vertex.pos.cast::<f32>().coords.into(); | |
| let norm_xyz: [f32; 3] = vertex.normal.cast::<f32>().into(); | |
| let pos_xyz_bytes: [u8; VERT_DIM_SIZE] = cast(pos_xyz); | |
| let norm_xyz_bytes: [u8; VERT_DIM_SIZE] = cast(norm_xyz); |
src/mesh/mod.rs
Outdated
| let vertex_f32 = (pos_xyz, norm_xyz); | ||
| let vertex_f32_bytes = (pos_xyz_bytes, norm_xyz_bytes); | ||
|
|
||
| if let Some(i_vertex) = vertices_hash.get(&vertex_f32_bytes) { | ||
| indices.push(*i_vertex); | ||
| } else { | ||
| vertices_hash.insert(vertex_f32_bytes, i_new_vertex); | ||
| vertices.push(vertex_f32); | ||
|
|
||
| indices.push(i_new_vertex); | ||
|
|
||
| i_new_vertex += 1; | ||
| } |
There was a problem hiding this comment.
The current implementation performs a get and then potentially an insert on the HashMap, which results in two lookups for new vertices. Using the entry API is more idiomatic and efficient as it performs only one lookup. This also makes the code more concise.
let vertex_f32_bytes = (pos_xyz_bytes, norm_xyz_bytes);
let index = *vertices_hash.entry(vertex_f32_bytes).or_insert_with(|| {
let new_index = i_new_vertex;
vertices.push((pos_xyz, norm_xyz));
i_new_vertex += 1;
new_index
});
indices.push(index);![recurseml[bot]](https://avatars.githubusercontent.com/in/1054195?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by RecurseML
🔍 Review performed on 7674485..aed941a
✨ No bugs found, your code is sparkling clean
✅ Files analyzed, no issues (3)
• Cargo.lock
• Cargo.toml
• src/mesh/mod.rs
1 participant
I've found that in order to render triangulated meshes, it is necessary to do the following operations:
The end user could do this themselves but adding a function that does that for them would make things easier.
I added the
GraphicsMeshtype which contains all the info needed to render the mesh(not including texture data)and the
build_graphics_mesh()function to the Mesh type. It does perform a bit of calculation; it takes 40ms to convert around 119,988 triangles. But it uses about 40% less vram vs directly drawing triangles. It's not likely that this conversion function will be the bottleneck of a rendering pipeline.High-level PR Summary
This PR introduces a new
GraphicsMeshtype andbuild_graphics_mesh()method to simplify rendering of triangulated meshes. The function handles converting vertices fromf64tof32precision and removes redundant vertices by generating an indexed representation, which reduces VRAM usage by approximately 40% at the cost of a one-time conversion overhead (40ms for ~120k triangles). The changes add thebytemuckdependency for safe type casting between numeric types.⏱️ Estimated Review Time: 5-15 minutes
💡 Review Order Suggestion
Cargo.tomlCargo.locksrc/mesh/mod.rs