Dynamic meshes#251
Conversation
|
|
||
| /// Cast slice of some arbitrary type into slice of bytes. | ||
| pub unsafe fn cast_arbitrary_slice<T, U>(slice: &[T]) -> &[U] { | ||
| let bytes = size_of::<T>() * slice.len(); |
There was a problem hiding this comment.
This is terribly unsafe in so many ways, a comment should definitely mention that it's effectively equivalen to transmute. You are only using it on Copy types, so it should probably be typeguarded as such. That way we at least get rid of Drop related issues.
There was a problem hiding this comment.
Also intended as internal method. Shouldn't be pub.
| } | ||
| } | ||
|
|
||
| Ok(unsafe { cast_arbitrary_slice_mut(&mut vertices.bytes[bytes_range]) }) |
There was a problem hiding this comment.
I think this is a safety issue. We probably should annotate AsVertex trait as unsafe so it is a responsibility of implementer to make sure it's safe to represent it as a byte slice (basically enforce #[repr(C)]).
There was a problem hiding this comment.
It is always safe to represent any type as bytes.
There was a problem hiding this comment.
And should be also safe to represent back. Alignment should be preserved.
There was a problem hiding this comment.
No, it is not always safe. See this thread https://internals.rust-lang.org/t/pre-rfc-v2-safe-transmute/11431
In fact, there are crates that help with this today. See bytemuck. Notice that Pod trait is actually unsafe and has quite some safety rules attached to it.
| &mut self, | ||
| index: usize, | ||
| range: Range<u32>, | ||
| fill: V, |
There was a problem hiding this comment.
I would rename fill to default_value or something like that. I interpreted those arguments as if the function would fill the whole provided range with fill value.
| /// # Panics | ||
| /// | ||
| /// This function will panic if wrong index type is requested. | ||
| pub fn modify_indices_16( |
There was a problem hiding this comment.
It seems to me that it would be better to have a single modify_indices that returns either &mut [u16] or &mut [u32], then possibly have those two methods use it. There is quite some duplication here.
Also, doc is inaccurate. It doesn't panic, it returns an error on mismatch.
| Ok(unsafe { cast_arbitrary_slice_mut(&mut indices.bytes[bytes_range]) }) | ||
| } | ||
|
|
||
| pub fn update(&mut self, queue: QueueId, factory: &Factory<B>) -> Result<(), UploadError> { |
There was a problem hiding this comment.
i think this should be called upload or flush. Also the upload should only really happen if it was updated since last time (it should have a dirty flag or something).
There was a problem hiding this comment.
It does use dirty regions
There was a problem hiding this comment.
Oh shit, you are totally right. I somehow skipped that in reading. Then it's only the naming.
2 participants
No description provided.