Skip to content

Assets::get_many #19484

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
Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Assets::get_many #19484

wants to merge 10 commits into from
+63 −1

Conversation

Wuketuke
Copy link
Contributor

@Wuketuke Wuketuke commented Jun 3, 2025

fixes #16244

Assets<A> should have get_many / get_many_mut functions just like Query<...> does.
thats why I took the implementation of get_many_mut for Query as an inspiration.

i had to add an #[allow(unsafe_code)] above get_many_mut, so i want to know if thats permitted at that location

this pr also makes #16334 obsolete

Testing

I added two unit tests for get_many_mut (since get_many is trivial), but ill happily write more if there are some edge cases i overlooked (unsafe code is scarry)

@alice-i-cecile alice-i-cecile changed the title Issue 16334 asset get many Assets::get_many Jun 4, 2025
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Assets Load files from disk to use for things like images, models, and sounds X-Contentious There are nontrivial implications that should be thought through D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 4, 2025
@alice-i-cecile
Copy link
Member

I think we should do this; Contentious is just for the use of unsafe (which I also think we should use here!).

@Wuketuke
Copy link
Contributor Author

i dont understand why the ci is failing now
i just merged 1 change from main into my pr, and havent touched the places where these errors get thrown now
it passed just fine before the merge

@andriyDev
Copy link
Contributor

This is broken at head currently https://discord.com/channels/691052431525675048/692572690833473578/1381857479520161803

I believe we're still trying to work out a fix.

@andriyDev
Copy link
Contributor

Related issue: #19573

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Unsafe Touches with unsafe code in some way S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Assets<T> should have get_many / get_many_mut or equivalent functions
3 participants
@Wuketuke @alice-i-cecile @andriyDev