Skip to content

Assets::get_many#19484

Open
Wuketuke wants to merge 10 commits into
bevyengine:mainfrom
Wuketuke:issue-16334-asset-get-many
Open

Assets::get_many#19484
Wuketuke wants to merge 10 commits into
bevyengine:mainfrom
Wuketuke:issue-16334-asset-get-many

Conversation

@Wuketuke

@Wuketuke Wuketuke commented Jun 3, 2025

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

Related issue: #19573

@Affinator

Copy link
Copy Markdown

What is the status of this? I could really use this for updating multiple assets in parallel...
How about adding it to the next milestone?

@andriyDev

Copy link
Copy Markdown
Contributor

@Affinator with a little luck, we'll get #22939 in 0.20, and we'll get get_many for free through Query's implementation. I'll make sure to add that to the list of follow-up work for that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

Assets<T> should have get_many / get_many_mut or equivalent functions

5 participants