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

gpui: Add drop_image #19772

Merged
merged 6 commits into from
Nov 22, 2024
Merged

gpui: Add drop_image #19772

merged 6 commits into from
Nov 22, 2024

Conversation

143mailliw
Copy link
Contributor

This PR adds a function, WindowContext::drop_image, to manually remove a RenderImage from the sprite atlas. In addition, PlatformAtlas::remove was added to support this behavior. Previously, there was no way to request a RenderImage to be removed from the sprite atlas, and since they are not removed automatically the sprite would remain in video memory once added until the window was closed. This PR allows a developer to request the image be dropped from memory manually, however it does not add automatic removal.

Release Notes:

  • N/A

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Oct 25, 2024
@143mailliw
Copy link
Contributor Author

It's worth noting that this can be kind of painful depending on use case, since on_release does not provide the WindowContext (for good reason, I suspect, unfortunately), and thus you cannot simply drop the image when the View is released.

@143mailliw 143mailliw marked this pull request as draft October 25, 2024 23:48
@143mailliw
Copy link
Contributor Author

I actually don't think it's possible to remove sprites from the atlas with the current structure of this code. This approach doesn't work, since the IDs change, and I can't think of one that would allow for this to work without substantially rewriting the atlas implementations.

@143mailliw 143mailliw closed this Oct 25, 2024
@143mailliw
Copy link
Contributor Author

I've rewritten parts of BladeAtlas and MetalAtlas to allow for pages to be deleted from the atlas.

I thought of two different solutions to this problem, and they both implement different ones. BladeAtlas uses FxHashMap instead of a Vec to index the pages, and MetalAtlas uses Vec<Option> instead of Vec. Both allow for pages to be removed without affecting other indexes.

The way I see it, the HashMap option (seen in BladeAtlas) has cleaner code but IDs can never be reused and performance is worse since the pages are now unordered. The Vec of Options maintains the page order, and IDs can be reused, but adds a fair bit of boilerplate code. I'm looking for feedback on this.

The other change that was made was counting allocations. Allocating space in a page now increments a counter, and removing the associated sprite decrements the counter. Note that removing the sprite does not deallocate it in the sprite map since BucketedAtlasAllocator does not allow for reuse of allocated space unless all items in the texture are deallocated, but since the texture is deleted when all items in the texture are dropped, there is no point deallocating each sprite.

The result of this is that the function cx.drop_image now works properly without affecting any other sprites.

@143mailliw 143mailliw reopened this Oct 26, 2024
@143mailliw 143mailliw changed the title gpui: add drop_image gpui: Add drop_image Oct 26, 2024
@143mailliw 143mailliw marked this pull request as ready for review November 2, 2024 02:46
@143mailliw
Copy link
Contributor Author

I decided to use a Vec<Option<T>>. I considered using a SlotMap (from slotmap), but it didn't seem fit for purpose.

The PR should be good to merge now, as far as functionality goes. I don't get any warnings on it, either.

@mikayla-maki mikayla-maki self-assigned this Nov 21, 2024
osiewicz and others added 2 commits November 21, 2024 19:14
Notably, we've reverted the texture index reuse, as it would impose cost upon callers who do not
remove images from the atlas.

Co-authored-by: Mikayla Maki <mikayla@zed.dev>
@143mailliw
Copy link
Contributor Author

143mailliw commented Nov 21, 2024

I have some concerns about not reusing atlas pages. I agree it's slower to reuse them, but not reusing them limits the amount of images that can ever be loaded.

I could see this being a problem for something like gpui-gradient, which creates an image every frame.

@mikayla-maki
Copy link
Contributor

@143mailliw I don't see us having any limits on the length of the textures array, but I thought it would probably be good practice to keep those arrays dense anyway. This latest commit should be ready to go, could you test it out in your application and make sure it's still working well?

@143mailliw
Copy link
Contributor Author

@mikayla-maki LGTM, tested and works as expected.

@mikayla-maki mikayla-maki merged commit ca76948 into zed-industries:main Nov 22, 2024
11 checks passed
Anthony-Eid pushed a commit to Anthony-Eid/zed that referenced this pull request Nov 22, 2024
This PR adds a function, WindowContext::drop_image, to manually remove a
RenderImage from the sprite atlas. In addition, PlatformAtlas::remove
was added to support this behavior. Previously, there was no way to
request a RenderImage to be removed from the sprite atlas, and since
they are not removed automatically the sprite would remain in video
memory once added until the window was closed. This PR allows a
developer to request the image be dropped from memory manually, however
it does not add automatic removal.

Release Notes:

- N/A

---------

Co-authored-by: Piotr Osiewicz <24362066+osiewicz@users.noreply.github.com>
Co-authored-by: Mikayla Maki <mikayla@zed.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants