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

Let Memory store arbitrary data for custom widgets. #255

Closed
optozorax opened this issue Mar 26, 2021 · 11 comments
Closed

Let Memory store arbitrary data for custom widgets. #255

optozorax opened this issue Mar 26, 2021 · 11 comments
Labels
feature New feature or request

Comments

@optozorax
Copy link
Contributor

optozorax commented Mar 26, 2021

Is your feature request related to a problem? Please describe.
I have this widget:
image image

This is the editor where the result can be previewed. And this widget has two states: View or Edit, this can be represented as bool. I believe this is more convenient to store this data in egui::Memory because this whole thing looks like some complicated widget that receives &mut String, and using this we can have the same benefits as other widgets have (able to serialize UI memory between runs). Also, creating custom memory and drag it into every function is quite inconvenient.

I looking for the code of CollapsingHeader, because it has some bool state too (opened or not). And found that Memory hard-coded all data for collapsing headers and collapsing header uses this private access. And, as I see there is no public interface to store data for a custom widget.

Describe the solution you'd like
First, we can provide an interface to store just a single bool, i32, String using enums for our id.

But a more scalable approach is using HashMap with Box<dyn Any + ...>. Using it, we can store arbitrary types for our id. And default widgets can be painlessly rewritten using it.

We also can require Serialize + Deserialize traits for all objects that should be stored in this type-map (under feature, of course).

But this type-map can possibly add visible runtime overhead comparing to current hard-coded maps.

Describe alternatives you've considered
Not allowing a user to store data in egui::Memory.

Additional context
I want to try to implement this feature by myself and open a Pull Request. What do you think?

@optozorax optozorax added the feature New feature or request label Mar 26, 2021
@optozorax
Copy link
Contributor Author

optozorax commented Mar 26, 2021

So, I tried to implement described idea with Box<dyn Any + 'static>, the result can be found here: optozorax@7db30a0.

Current observations:

  • Using Box<dyn Any> is very ugly. Even cloning done by miracle ugly hacks.
  • This works until we need Serialization and Deserialization. Serialization is in some way possible using ugly hacks (storing String and using serde_json), but Deserialization is impossible. (maybe possible, but very very ugly and unsafe https://github.com/alecmocatta/serde_traitobject)

So, the idea with any-map is failed, so I suggest it's better to provide the user a way to store their data using simple Rust enums, maybe also introducing helper functions/macros to get/set data from that enums, or parsing using if let.

@optozorax
Copy link
Contributor Author

optozorax commented Mar 26, 2021

I tried to sleep, but this idea came: deserialization can be deferred, and can be performed when we call get function with specialized type.

@optozorax
Copy link
Contributor Author

optozorax commented Mar 27, 2021

So, I created a draft PR where you can view how the approach with Any may look like. And made CollapsingHeader using this. This seems to work.

Pros:

  • Nice interface for a user.

Cons:

  • Ugly and complicated internal code.
  • After removing all unwraps error handling will become very complicated. Maybe this is not important.
  • Some deserialization errors can be determined only after some time using the interface.
  • New dependency: serde_json (for persistence feature).
  • Now we can't reset individual types (because it can be loaded from a file, and stored in ToDeserialize state), only everything can be reset.
  • Individual types can't be counted (same reason).

Another approach is introduce something like this type:

enum DataStorage {
  None,
  Bool(bool),
  Int(i64),
  Float(f32),
  String(String),
  Tuple(Box<DataStorage>, Box<DataStorage>)
}

Pros:

  • Nice internal code without ugly hacks.
  • Straightforward cloning, serialization, and deserialization.
  • No extra dependencies.

Cons:

  • Ugly interface for a user, now they have to parse this struct by themselves using match and if let every time.
  • Some deserialization errors can be determined only after some time using the interface too because a user can expect something like Tuple(Float(_), Bool(_)), but now there is stored Tuple(Float(_), Tuple(Bool(_), Bool(_))) because a version of their code changed between runs.
  • Now we also can't reset individual types (because we can't determine if type Bool(_) refers to our internal widget or to user Bool(_)), only everything can be reset.
  • Individual types can't be counted (same reason).

Now I see that enum-approach has the same cons as Any-approach. And I think that a nice user interface is more important, so Memory should be implemented using Any, and we should pay some cost using it (remove the ability to count and reset individual widgets).

@optozorax
Copy link
Contributor Author

optozorax commented Mar 27, 2021

I said:

After removing all unwraps error handling will become very complicated.

But this is not quite true. Why user should care if some internal temporary UI data is not saved correctly between runs of different versions of their program? I think a user should use only functions get_or_insert(Id, T) -> &T, get_or_default(Id) -> &T, and insert(Id, T) and that's all. They can't return Option<> or can't panic because they will insert provided/default value if some error happens. With this approach and this interface we can stop bothering about error origin: downcast error, deferred deserializing error, we can just insert the correct new/default value.

But this is just an idea. And possibly this can create some obscure and hard-to-find errors for complicated interfaces.

@optozorax
Copy link
Contributor Author

I believe this feature can be done in two PRs:

  1. Add API to Memory to store arbitrary data. Rewrite system/default widgets and containers to use this API.
  2. Add an example on how to use this API. Maybe view/edit widget from the first message should suit.

Default widgets should be rewritten to use this API because users may rely on the source code of these widgets.

(I still want to implement this if you give approval)

@optozorax optozorax changed the title Let memory store arbitrary data for custom widgets. Let Memory store arbitrary data for custom widgets. Mar 27, 2021
@optozorax
Copy link
Contributor Author

The downcast error may occur when one Id is used for different widgets. Maybe this error should be logged or provided to a user as Result<> 🤔

@emilk
Copy link
Owner

emilk commented Mar 27, 2021

Wow!

I went through exactly this thought process many months ago, but I never managed to crack the nut - but it seems you did!

Postponing deserialization until we know what type to deserialize to is really clever.

As for errors: this storage should only be used for things that are non-critical ("the selected tab" etc), so I think silently ignoring errors is fine (*). Of course we need to be very clear documenting this.


(*) it may also be about time that egui got some way to report errors. Either using the log crate, or using some sort of message system (part of the Output struct), but that is a topic for another issue.

@emilk
Copy link
Owner

emilk commented Mar 27, 2021

As for JSON: I use it elsewhere, but I am considering switching it out for something that supports f32::INFINITY etc. Maybe ron

@parasyte
Copy link
Contributor

parasyte commented Apr 1, 2021

Using ron is quite a great idea. You can use ron::value::Map directly to store the state without building any bespoke dynamic types. I was considering json::object::Object but if you don't have any hard dependency on JSON as a serialization format, then there are loads of other alternatives to consider, and ron is a top contender.

Also, it's worth pointing out that serde supports transcoding between formats, so ron values can be transcoded to JSON for persistence.

@optozorax
Copy link
Contributor Author

I started to implement an example widget about how to use Memory in custom widgets.

image

Now I need to write a bunch of comments and wait for #257 to be merged.

@emilk emilk closed this as completed in 186362a Apr 12, 2021
@emilk
Copy link
Owner

emilk commented Apr 12, 2021

I just used this for the first time in daf2e13 - I think this is a good example of something that does not belong in the user state, and so egui can help you keep track of instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants