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.

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

Remove redundant texture copies in TextureCopyNode #871

Merged
merged 4 commits into from
Nov 17, 2020

Conversation

4-rodrigo-salazar
Copy link
Contributor

@4-rodrigo-salazar 4-rodrigo-salazar commented Nov 16, 2020

For each AssetEvent::Modified event, TextureCopyNode will do 1 copy from each Texture into a buffer in the render_context. Every time a system calls Assets.get_mut to retrieve a Texture from a Handle it will trigger the AssetEvent::Modified event. If you have multiple systems which need to retrieve the same texture, this causes TextureCopyNode to spend time copying the data for the same texture multiple times, once for each call to get_mut that happened for that Texture (as well as the time spent copying, it also raises the peak memory). A Gist which contains sample code which uses the same Texture from multiple systems: https://gist.github.com/rod-salazar/f45608644f042de036c09bf2565aba69

This change makes it so that we only do the copy once per texture handle.

The assumption this change makes is that we only need to synchronize between the a particular Texture's data and the render_context once since this TextureCopyNode happens after the calls to get_mut.

I ran into this scenario in a different way though. When I was writing some code which attempted to call texture.get_mut in a loop, where sometimes I would ask for the same texture multiple times. In this case, my loop caused hundreds of copies for what I thought was just a harmless lookup in the assets using get_mut.

@4-rodrigo-salazar
Copy link
Contributor Author

Gonna do some more testing and will re-open later

@4-rodrigo-salazar
Copy link
Contributor Author

Okay, meant to continue, not return, fixed

@cart
Copy link
Member

cart commented Nov 17, 2020

Very good call. Thanks!

@cart cart merged commit 3fca8c6 into bevyengine:master Nov 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants