Index buffer specialization (Bevy PR #568)

[guimcaballero]

Bevy PR #568 reworked mesh indices, this PR introduces a fix to deal with that.

There's one thing I don't like of my implementation, because we would now have the same code twice and that's a big no no.

The other option is to use something like this above the for index in indices.chunks(3) loop:

let indices: Vec<usize> = match indices {
                        Indices::U16(vector) => {
                            vector.iter().map(|index| *index as usize).collect()
                        }
                        Indices::U32(vector) => {
                            vector.iter().map(|index| *index as usize).collect()
                        }
                    };

But as far as I know this would make a copy of the whole indices vector, and that doesn't seem right either.

Any ideas on how to do this in a more idiomatic way I'm not seeing?

[aevyrie]

Thanks for the fix!

But as far as I know this would make a copy of the whole indices vector, and that doesn't seem right either.
Any ideas on how to do this in a more idiomatic way I'm not seeing?

Perhaps the entirety of world_triangle construction could be put into a function that accepts any Vec<T> where T: Into<usize>?

Then the code might look something like:

pick_intersection = match indices {
    Indices::U16(vector) => ray_mesh_intersection(&pick_ray, &vector),
    Indices::U32(vector) => ray_mesh_intersection(&pick_ray, &vector),
}

Then, regardless of index type, it can be converted into a usize when building the triangle:

for i in 0..3 {
    world_vertices[i] = mesh_to_world.transform_point3(Vec3::from(
        vertex_positions[index[i] as usize],
    ));
}

The other benefit of this is we could more easily write some unit tests for this function!

[aevyrie]

Looking at this code again, I think we could make another improvement (while you're at it!). Every group of three indices is being bounds checked if index.len() != 3 { break; }. Instead, I think a better solution would just check that the input vector is divisible by 3: vector.len() % 3 == 0

[guimcaballero]

Sounds good!

Just one thing, we can't use as usize inside the function, because index[i] is T, which isn't a primitive and rust doesn't let us use as. So instead I used try_into(), because usize doesn't implement From<u32>, it only implements TryFrom<u32>.

Also, I'm unwrapping the try_into() with unwrap_or_default(), because the Error doesn't implement Debug so we can't use unwrap(). What are your thoughts on using default? I don't know if panicking would be better instead of defaulting, since it would make the error more obvious.

[aevyrie]

Just one thing, we can't use as usize inside the function, because index[i] is T, which isn't a primitive and rust doesn't let us use as. So instead I used try_into(), because usize doesn't implement From<u32>, it only implements TryFrom<u32>.

Okay!

What are your thoughts on using default? I don't know if panicking would be better instead of defaulting, since it would make the error more obvious.

I think panicking would be better. I'd prefer we crash the plugin instead of sending bad data into the picking algorithm, it will be much easier to debug if it ever fails to convert. As far as I can tell, this will only happen on 16-bit architectures where usize is not large enough to hold a u32.

[guimcaballero]

Great, changed!