Skip to content

Commit

Permalink
Merge #219
Browse files Browse the repository at this point in the history
219: Revert PR #197 and re-implement to fix incorrect buffer writes r=frizi,omni-viral a=jaynus

#197  unintentionally allowed for incorrectly writing `Vec` types when collected and submitted. This was discussed in the PR and actually seen, where a crash was mentioned, but it was *incorrectly* assumed to be an upstream problem. The change in #197 caused `upload_*` functions to accept a plain `Vec` as `&T` can AsRef a `Vec`, which was then cast incorrectly to a u8 and submitted. 

This was only evident in the `quads` example because it is the only example which actually provides a `Vec` type (from a `Vec::collect` call) for a buffer upload.

This PR reverts #197  and changes the fix for #58 to the original concept of just bounding T: 'static + Copy, instead of changing from `&[T]` to `&T`. As discussed in #197, this will have problems with large arrays being submitted - but we can just ignore that, as its unrealistic; it is better to *correctly* accept `Vec` types for large arrays, instead of compensating for large static slices which will almost never exist.

Co-authored-by: jaynus <jaynus@gmail.com>
  • Loading branch information
bors[bot] and jaynus authored Oct 1, 2019
2 parents 90ea766 + 7190057 commit 6359807
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 20 deletions.
30 changes: 12 additions & 18 deletions factory/src/factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -398,24 +398,19 @@ where
&self,
buffer: &mut Buffer<B>,
offset: u64,
content: &T,
content: &[T],
) -> Result<(), MapError>
where
T: ?Sized + 'static,
T: 'static + Copy,
{
let content_size = std::mem::size_of_val(content);

let content = std::slice::from_raw_parts(
{
let content_ptr: *const T = content;
content_ptr as *const u8
},
content_size,
content.as_ptr() as *const u8,
content.len() * std::mem::size_of::<T>(),
);

let mut mapped = buffer.map(&self.device, offset..offset + content_size as u64)?;
let mut mapped = buffer.map(&self.device, offset..offset + content.len() as u64)?;
mapped
.write(&self.device, 0..content_size as u64)?
.write(&self.device, 0..content.len() as u64)?
.write(content);
Ok(())
}
Expand Down Expand Up @@ -445,17 +440,16 @@ where
&self,
buffer: &Buffer<B>,
offset: u64,
content: &T,
content: &[T],
last: Option<BufferState>,
next: BufferState,
) -> Result<(), UploadError>
where
T: ?Sized + 'static,
T: 'static + Copy,
{
assert!(buffer.info().usage.contains(buffer::Usage::TRANSFER_DST));

let content_size = std::mem::size_of_val(content) as u64;

let content_size = content.len() as u64 * std::mem::size_of::<T>() as u64;
let mut staging = self
.create_buffer(
BufferInfo {
Expand Down Expand Up @@ -556,20 +550,20 @@ where
image_layers: SubresourceLayers,
image_offset: image::Offset,
image_extent: Extent,
content: &T,
content: &[T],
last: impl Into<ImageStateOrLayout>,
next: ImageState,
) -> Result<(), UploadError>
where
T: ?Sized + 'static,
T: 'static + Copy,
{
assert!(image.info().usage.contains(image::Usage::TRANSFER_DST));
assert_eq!(image.format().surface_desc().aspects, image_layers.aspects);
assert!(image_layers.layers.start <= image_layers.layers.end);
assert!(image_layers.layers.end <= image.kind().num_layers());
assert!(image_layers.level <= image.info().levels);

let content_size = std::mem::size_of_val(content) as u64;
let content_size = content.len() as u64 * std::mem::size_of::<T>() as u64;
let format_desc = image.format().surface_desc();
let texels_count = (image_extent.width / format_desc.dim.0 as u32) as u64
* (image_extent.height / format_desc.dim.1 as u32) as u64
Expand Down
2 changes: 1 addition & 1 deletion mesh/src/mesh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ impl<'a> MeshBuilder<'a> {
factory.upload_buffer(
&mut buffer,
0,
&**indices,
&indices,
None,
BufferState::new(queue)
.with_access(gfx_hal::buffer::Access::INDEX_BUFFER_READ)
Expand Down
2 changes: 1 addition & 1 deletion rendy/examples/quads/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ where
.upload_buffer(
posvelbuff,
0,
&POSVEL_DATA[..],
&POSVEL_DATA,
None,
BufferState {
queue: QueueId {
Expand Down

0 comments on commit 6359807

Please sign in to comment.