tgfrerer / island

🌋🐎 Project Island is an experimental, hot-reloading Vulkan Renderer for Linux and Windows, written in C/C++.
MIT License
1.04k stars 42 forks source link

[le_mesh_generator] Redundant code #64

Closed orion160 closed 1 month ago

orion160 commented 1 month ago

https://github.com/tgfrerer/island/blob/wip/modules/le_mesh_generator/le_mesh_generator.cpp

line 184

    uint32_t num_bytes_per_index = ( p_num_bytes_per_index ) ? *p_num_bytes_per_index : 0;
    void*    index_data          = le_mesh::le_mesh_i.allocate_index_data( mesh, num_indices, &num_bytes_per_index );

    if ( p_num_bytes_per_index ) {
        *p_num_bytes_per_index = num_bytes_per_index;
    }
tgfrerer commented 1 month ago

yes this looks a bit confusing indeed - thank you very much for spotting this...

The reason why i'm setting p_num_bytes_per_index explicitly is because this is a in-out variable, and callers of the function may use it to question the number of bytes per index as a side-effect of calling le_mesh_generator_generate_sphere.

If the user of the function provides a valid pointer, i take the hint of numbers of bytes per index as given by the user to allocate the mesh. Because the allocator may change bytes per index i then write this back to the user-given pointer, so that the user can check whether the number of bytes per index has changed from what they hinted at.

It's possible that the number of bytes required per index may not be what the user expects because a mesh with more than 65536 indices can't use a 16 bit index type anymore, and must use a 32bit type.

Maybe i should add a comment explaining what the function does, but on the whole I would think what it does is correct, unless I didn't spot another issue...

orion160 commented 1 month ago

Oh, thank you.

I didn't spot the &num_bytes_per_index at allocate_index_data