Skip to content

Change popup memory to be per-viewport#6753

Merged
lucasmerlin merged 1 commit intoemilk:masterfrom
mkalte666:popup_memory_per_viewport
Apr 22, 2025
Merged

Change popup memory to be per-viewport#6753
lucasmerlin merged 1 commit intoemilk:masterfrom
mkalte666:popup_memory_per_viewport

Conversation

@mkalte666
Copy link
Copy Markdown
Contributor

Starting with 77244cd the popup open-state is cleaned up per memory pass.

This becomes problematic for implementations that share memory between viewports (i.e. all of them, as far as i understand it), because each viewport gets a context pass, and thus a memory pass, which cleans out popup open state.

To illustrate my issue, i have modifed the multiple viewport example to include a popup menu for the labels: https://gist.github.com/mkalte666/4ecd6b658003df4c6d532ae2060c7595 (changes not included in this pr).

Then, when i try to use the popups, i get this:

before.mp4

Immediate viewports just break popup handling in general, while deferred viewports kinda work, or dont. In this example ill be honest, it kind of still did, sometimes. In my more complex app with multiple viewports (where i encountered this first) it also just broke - even when just showing root and one nother. Probably to do with the order wgpu vs glow draws the viewports? Im not sure. In any case:

This commit adds Memory::popup (now Memory::popups) to the per-viewport state of memory, including viewport aware cleanup, and adding it to the list of things cleaned if a viewport goes away.

This results in the expected behavior:

after.mp4

I will note that with this, changing focus does not cause a popup to be closed, which is consistent with current behavior on a single app.

Hope this helps

~Malte

Starting with 77244cd (emilk@77244cd) the popup open-state is cleaned up per memory pass.

This becomes for implementations that share memory between viewports,
because each viewport gets a context pass, and thus a memory pass, which
cleans out popup open state.

This commit adds `Memory::popup` (now `Memory::popups`) to the
per-viewport state of memory, including viewport aware cleanup, and
adding it to the list of things cleaned if a viewport goes away.
@github-actions
Copy link
Copy Markdown

Preview available at https://egui-pr-preview.github.io/pr/6753-popupmemoryperviewport
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

Copy link
Copy Markdown
Collaborator

@lucasmerlin lucasmerlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think a similar bug could also exsit for menus, where I also do some cleanup if the memory wasn't shown for a frame. But it's implemented using the frame count there, so maybe it's fine.

@lucasmerlin lucasmerlin added bug Something is broken egui labels Apr 16, 2025
@lucasmerlin lucasmerlin merged commit 114460c into emilk:master Apr 22, 2025
26 of 27 checks passed
darkwater pushed a commit to darkwater/egui that referenced this pull request Aug 24, 2025
Starting with 77244cd the popup
open-state is cleaned up per memory pass.

This becomes problematic for implementations that share memory between
viewports (i.e. all of them, as far as i understand it), because each
viewport gets a context pass, and thus a memory pass, which cleans out
popup open state.

To illustrate my issue, i have modifed the multiple viewport example to
include a popup menu for the labels:
https://gist.github.com/mkalte666/4ecd6b658003df4c6d532ae2060c7595
(changes not included in this pr).

Then, when i try to use the popups, i get this: 


https://github.com/user-attachments/assets/7d04b872-5396-4823-bf30-824122925528

Immediate viewports just break popup handling in general, while deferred
viewports kinda work, or dont. In this example ill be honest, it kind of
still did, sometimes. In my more complex app with multiple viewports
(where i encountered this first) it also just broke - even when just
showing root and one nother. Probably to do with the order wgpu vs glow
draws the viewports? Im not sure. In any case:

This commit adds `Memory::popup` (now `Memory::popups`) to the
per-viewport state of memory, including viewport aware cleanup, and
adding it to the list of things cleaned if a viewport goes away.

This results in the expected behavior:



https://github.com/user-attachments/assets/fd1f74b7-d3b2-4edc-8dc4-2ad3cfa2160e

I will note that with this, changing focus does not cause a popup to be
closed, which is consistent with current behavior on a single app.

Hope this helps 

~Malte 

<!--
Please read the "Making a PR" section of
[`CONTRIBUTING.md`](https://github.com/emilk/egui/blob/master/CONTRIBUTING.md)
before opening a Pull Request!

* Keep your PR:s small and focused.
* The PR title is what ends up in the changelog, so make it descriptive!
* If applicable, add a screenshot or gif.
* If it is a non-trivial addition, consider adding a demo for it to
`egui_demo_lib`, or a new example.
* Do NOT open PR:s from your `master` branch, as that makes it hard for
maintainers to test and add commits to your PR.
* Remember to run `cargo fmt` and `cargo clippy`.
* Open the PR as a draft until you have self-reviewed it and run
`./scripts/check.sh`.
* When you have addressed a PR comment, mark it as resolved.

Please be patient! I will review your PR, but my time is limited!
-->

* Closes <https://github.com/emilk/egui/issues/THE_RELEVANT_ISSUE>
* [x] I have followed the instructions in the PR template
* [x] ~~I have run check.sh locally~~ CI on the fork, including checks,
went through.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something is broken egui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants