Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Request: T: 'static + Copy bound on Factory's upload_* functions #58

Closed
mtak- opened this issue Feb 28, 2019 · 3 comments · Fixed by #197
Closed

Request: T: 'static + Copy bound on Factory's upload_* functions #58

mtak- opened this issue Feb 28, 2019 · 3 comments · Fixed by #197
Labels
diff: easy enhancement New feature or request good first issue Good for newcomers

Comments

@mtak-
Copy link

mtak- commented Feb 28, 2019

I just ran into an issue where I was accidentally uploading some references to the GPU - passing in &[some_ref] to upload_visible_buffer. The 'static bound would have prevented that error.
@zakarumych zakarumych added enhancement New feature or request good first issue Good for newcomers diff: easy labels May 20, 2019
@malobre
Copy link
Contributor

malobre commented Aug 30, 2019

I'll try working on this before next week.

@malobre malobre mentioned this issue Aug 30, 2019
Merged
@zakarumych
Copy link
Member

I would advice against Copy bound since large arrays don't implement it.

@zakarumych
Copy link
Member

But uploading non-static data, while technically valid, makes little sense

bors bot added a commit that referenced this issue Sep 23, 2019
@bors
Merge #197
b48f5d1 
197: Fixes #58 + minor fixes r=Frizi a=malobre

Fixes #58 + minor changes:
* The `source_shaders` example had incorrect logging filter
* The `quads` example used `println!` instead of logging-macros


Co-authored-by: Maël Obréjan <malobre@armanix.net>
@bors bors bot closed this as completed in 1a23b74 Sep 23, 2019
jaynus added a commit to jaynus/rendy that referenced this issue Sep 30, 2019
@jaynus
Revert "Fixes amethyst#58 + minor fixes"
50aa6b1 
This reverts commit 1a23b74.
@jaynus jaynus mentioned this issue Sep 30, 2019
Merged
bors bot added a commit that referenced this issue Oct 1, 2019
@bors @jaynus
Merge #219
6359807 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
diff: easy enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixes #58 + minor fixes
3 participants
@malobre @zakarumych @mtak-