Group created values in IdTypeMap by a shared Id that can later be used to remove them in bulk.

[AlexanderSchuetz97]

Hello,
this is my first PR to this repository.

I hope it meets your requirements on how a PR is supposed to be filed.

It is a small change that allows me to remove temporary values stored in the TypeIdMap created by a part of the ui that is obsolete.

What is the problem?
I have a very generic UI that changes drastically based on data/state and user actions.
It is impossible for me to uniquely id many of my widgets that require an id.
I have therefore opted to assign semi random id's together with the state.
This works great, however sometimes user action or data/state changes cause parts of the ui
to disappear and different stuff to be added.
Over a long time this causes the TypeIdMap to bloat with temporary values.
To print the count I used

  ctx.data_mut(|m | {
      println!("{}", m.len());
  });

In my update loop. This value never decreased. (Memory Leak)

How does this PR help me?
This pr adds a fn to Context that I can call to assign a id to all elements inserted
into the TypeIdMap during the execution of a closure. (Similar to ui::push_id)
The "group" id is determined at the time of insertion into the TypeIdMap.
During this closure I will update my UI. (For example update/create a nested left or right Panel and all elements in it).
This allows me to logically group my elements so that I can later remove all temporary values associated to the elements once
I desire them to become obsolete.

What alternatives have I considered?
I was unable to individually remove all the elements from the TypeIdMap because
I was not able to correctly deduce the TypeId which is part of the Key Hash.
Calling TypeIdMap::clear() is also not workable for me because it makes the entire UI jerk back to default settings,
undoing any resizing the user may have done to still relevant panels.

Naming/Bikeshedding
I am not at all attached to the function name "push_data_id" or the name "data_id"
I gave this name group id explained above which is stored for every value in the TypeIdMap.
As you are probably aware naming things is not trivial.
If you want me to change the name to something else then tell me and I will gladly change it.

Since this is quite a technical PR I don't think adding any screenshots provides value.
If you want me to make a simple app demonstrating my problem (by printing the number) and making a button that appears/disappears a panel each time with a new id then I can do so. But once again since this is a technical issue I think such an app would be boring to look at?

I would be delighted if this could be merged.

Sincerely
Alexander Schütz

[lucasmerlin]

I agree that this is a problem, but I'm not sure if this is the best way of fixing this. I think it'd be neat if there was some way to automatically clean up data that e.g. wasn't accessed last frame or for the last N frames or seconds. Would that also fix things for your usecase?

[github-actions]

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

[AlexanderSchuetz97]

No, last n frames/seconds is not a good fix for me because I also hide stuff temporarily and later show it again. Your suggestion would remove this hidden stuff aswell, which is not desired.

An example of what my app has to do:
Hide right panel id 1 for 10 minutes and show right panel id 2 instead. After 10 minutes state/change or user input causes me to discard right panel id 2 and show right panel id 1 again. By hide panel you can assume that its literally if self.showPanel1 {self.updatePanel1(ui, ctx, ...)} in the update loop. So with your solution it would drop the data for panel1 after the timeout. It would be sad if the user had lunch and came back then continued their work only to have panel1 resize back to default when its shown again.

Id prefer it if I could manually choose when to discard.

Sidenote:
In your solution could be implemented additionally and opt in for people who desire it.
Something like:
ctx.enableTempCleanup(Some(Duration::fromSecs(10))
or pass a number of frames.
It doesnt conflict itself with what I implemented here.

[AlexanderSchuetz97]

@lucasmerlin I am having massive problems with your check script.
It doesn't appear to want to do its "thing" so sorry that this is trial and error ish, but In the future I think its a good idea to provide a dockerfile that can run these checks no? I have a pretty default Debian 12 with x11 running XFCE here.

cargo check --quiet -p eframe --no-default-features --features glow
I cant get past this error "error: The platform you're compiling for is not supported by winit"

I will fix the cargo fmt issue and hope for the best.

[emilk]

Have you looked at IdTypeMap::max_bytes_per_type? it should do the GC for you

[AlexanderSchuetz97]

@emilk That appeared to only works for persistet type. I dont have any persistence so its all temp values. I couldnt see how this setting affects the temp values. I will double check this later.

Even if it worked I again would like this level of fine control as mentioned removing the "oldest" or oldest unused value is not the correct behavior I desire.

[zhatuokun]

I agree that this is a problem, but I'm not sure if this is the best way of fixing this. I think it'd be neat if there was some way to automatically clean up data that e.g. wasn't accessed last frame or for the last N frames or seconds. Would that also fix things for your usecase?

@lucasmerlin egui::cache::FramePublisher is doing this, but as mentioned in #5441, If you display multiple viewports, it cannot work as expected. I suspect that having multiple viewports open is causing the cleanup code to be called multiple times, but I haven’t been able to locate the relevant code. Perhaps you could try to fix it.

[AlexanderSchuetz97]

Hello @lucasmerlin @emilk
What is your decision in regards to merging this?
I have been using this fork for a week now and it works as expected. I am unsure on how to interprete your feedback.

I really need this feature for my use case and would prefer to not have to maintain this as a patch that I constantly have to rebase onto all your changes.

Is there anything you still want me to do?

If you do not want to merge this then
I have in the meantime come up with an idea that would be less intrusive and would also permit me in solving this problem. The less intrusive idea would just add 2 fn's. 1. to iterate over the raw keys (u64) of TypeIdMap and 2. to remove from the TypeIdMap based on raw key. We dont have to expose that its an u64, we can also, if you want wrap it in an opaque raw key type that is hash and eq. That would be sufficient for me. This would allow me to implement all my housekeeping on my end. If you dont want to merge my original solution then I ask you to please consider at least this proposal. If you are okay with this second proposal tell me and I will immideatly implement it and make a PR.

Sincerely
Alexander Schütz

[zhatuokun]

@AlexanderSchuetz97 Can egui::cache::FramePublisher solve your problem? It can retain custom data from the current frame to the next frame. If you don’t manually set the value in the next frame, it will be cleared. In other words, if you open a new page and store some custom data, the data will be automatically cleared from memory when you close the page (no more calls to the set method).

It is very suitable for storing values that will be automatically cleared when they are not needed.

// You can customize the types of keys and values.
type MyPublisher = egui::cache::FramePublisher<String, u32>;

fn foo(ui: &mut egui::Ui) {
        let key = String::from("random id");
        // In this use case, the first frame is `None`, and every subsequent frame is `Some(0)`.
        // If the set method is not called in the future, the stored data will be automatically cleared in the next frame.
        let prev_value = ui.ctx().memory_mut(|m| m.caches.cache::<MyPublisher>().get(&key).copied());

        // some other code

        // Manually set the values that need to be saved to the next frame.
        ui.ctx().memory_mut(|m| m.caches.cache::<MyPublisher>().set(key, 0));
}

[AlexanderSchuetz97]

@zhatuokun no it can not. As mentioned previously I have to hide a ui (i.e. its not drawn!) for significant amount of time and it later becomes visible again and should have the same properties. (For example positioning of a panel that was manually resized by the user) I dont manually set ANY data into the ctx.memory map its the Side Panel for example or a scroll pane that does so.
FramePublisher will remove the values the first frame they become invisible. Once the elements become visible again (based on state/user interaction) they will snap back to their default positions. This will massively annoy the user. Because of this FramePublisher is useless to me.

[zhatuokun]

@zhatuokun no it can not. As mentioned previously I have to hide a ui (i.e. its not drawn!) for significant amount of time and it later becomes visible again and should have the same properties. (For example positioning of a panel that was manually resized by the user) I dont manually set ANY data into the ctx.memory map its the Side Panel for example or a scroll pane that does so. FramePublisher will remove the values the first frame they become invisible. Once the elements become visible again (based on state/user interaction) they will snap back to their default positions. This will massively annoy the user. Because of this FramePublisher is useless to me.

Oh, I'm sorry, I ignored your answer above.

I have done similar things before, when I chose to create a new struct and manage it manually.

[emilk]

I think this is the wrong solution.

If you want fine control over when data is retained/removed I suggest you either store it outside of IdTypeMap, or we add add retain functions to IdTypeMap, and you can store the extra state inside of the type itself:

impl IdTypeMap {
	/// For each temp value of the given type, only retain it if the predicate is `true`.
	pub fn retain_temp<T>(&mut self, keep: impl Fn(&mut T)) {
	}
}

EDIT: Or do you need this to work for types you don't have control for too, e.g. CollapsingHeader?

[AlexanderSchuetz97]

@emilk
Your edit is correct, I do not store anything myself in the typeid map its things like panels, buttons labels etc that store things in there. My own state is in my app struct. The types in the typeidmap are impossible to guess for me (I tried for several hours) and I think most types are private of egui anyways.
I am afraid your proposed method would not let me clean this map of garbage.

[emilk]

I'm hesitant to add complexity to egui to support this somewhat niche use case.

However, we're in the process of adding a lookup-map for Id:s in this PR:

• Make debugging ids easier #5851

Theoretically we could use this map to look up all ancestor Id:s from some Id (e.g. that of a top-level panel), and then you could write code for "remove data for these Id"s.

WDYT?

[AlexanderSchuetz97]

That would work were if the typeids were not included in the hashes. Sadly they are. the ids are also a problem but predicting the typeids is harder or even impossible as you seem to feed private types into the map.

If you dont want to expose as much api bandwidth as this change proposes, expose the raw keys to the map u64? or wrap them in a type thats eq, clone and hash and allow me to iterate over all raw keys and remove by them.Then I could track the garbage on my end.

I already mentioned this in a comment above. I discussed this with @lucasmerlin in discord and we came to the conclusion that the original solution i proposed is probably best. If you want I can remove the changes from the context.rs and we can only keep the changes to the typeidmap. It wouldnt look as nice but it would still work and thats all I care about.