-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
gpu asset transfer priorities #22557
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
i'm unsure about the current |
25d8011 to
6eb5d96
Compare
# Objective while writing the tests for #22557 i noticed that the perf was much worse for failed materials compared to older bevy versions pre #19667. this is probably because of the allocations in `ErasedMaterialKey::new` which was called regardless of whether the material was ready or not. ## Solution fail faster by checking bindgroup (the only fallible part of the material preparation) before doing any other work. ## Testing using the `gpu_transfer_limits` example from #22557, the time spent preparing failing materials drops by ~80% <img width="744" height="346" alt="image" src="https://github.com/user-attachments/assets/5123dac7-b90b-41a4-b139-2cc3890fc05b" />
| /// Bytes written this frame. | ||
| pub bytes_written: AtomicUsize, | ||
| pub bytes_per_frame: RenderAssetBytesPerFrame, | ||
| bytes_written: bevy_platform::sync::RwLock< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this now private because people would be expected to use the new Stats struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, i felt exposing an RwLock was asking for deadlock trouble, and a bit obscure. there shouldn't be a reason to interact with it directly except for getting debug information anyway (the request_bytes and write_bytes functions are pub so the supported interactions can be accessed by user code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's what I expected, makes sense.
| fn extract_stats( | ||
| channel: Extract<Res<StatsChannel>>, | ||
| limiter: Res<RenderAssetBytesPerFrameLimiter>, | ||
| ) { | ||
| let stats = limiter.stats(); | ||
| let _ = channel.sender.send(stats); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the stats stuff should probably be more integrated with the Diagnostic system and transparently do that readback to main world.
This can be done in a separate PR though.
IceSentry
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a release note but other than that and the minor comments LGTM
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
|
Actually, it also needs a migration guide for all the assets that now need the new Priority stuff. |
|
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
|
i have a related pr that i hope to land this cycle too, i'll aim to combine the release notes at least, maybe the migration guide too. |
|
I would suggest at least adding the necessary files but keep them as a short description or even just a TODO |
a64c7ef to
acc59ba
Compare

Objective
RenderAssetBytesPerFramelimits per-frame gpu transfers to avoid frame hiccups, but doesn't provide any way to prioritise particular assets.also, when potential gpu-transfer delays are introduced, we may see flickering as old images/meshes/materials are unloaded and new ones are not immediately uploaded. when not using the gpu transfer throttle you can work around this by preloading the assets or by waiting for the server to signal the asset is loaded. with the throttle it's unavoidable. this can result in flickering e.g. for text, every time a new glyph is added to a font atlas.
Solution
RenderAssetTransferPrioritywith anImmediatevariant and aPriority(i16)variant.MeshandImageImmedateImmediateassets immediately regardless of bytes already transferred.RenderAssetTransferPriority::Immediate, so we can leave the existing asset until the new one is ready, avoiding flickering.staleflag toRenderAssetsand makeas_bind_grouptake only non-stale assets, to ensure that materials don't get processed when the image they rely on is not yet available, but an older version is still availableTesting
this is a forward-port of code i have built against bevy 16, so there may be issues. i need to write a proper example/test for it.added
gpu_transfer_limitsexample to demonstrate.