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

Arbitrary data in Memory using Any, fix #255 #257

Merged
merged 18 commits into from
Apr 12, 2021

Conversation

optozorax
Copy link
Contributor

@optozorax optozorax commented Mar 27, 2021

@optozorax optozorax changed the title Arbitrary data in memory using Any Arbitrary data in Memory using Any Mar 27, 2021
egui/src/memory.rs Outdated Show resolved Hide resolved
egui/src/memory.rs Outdated Show resolved Hide resolved
@optozorax
Copy link
Contributor Author

Tomorrow I will write more documentation and rewrite all widgets to new API.

egui/src/memory.rs Outdated Show resolved Hide resolved
egui/src/memory.rs Show resolved Hide resolved
@optozorax
Copy link
Contributor Author

I will test this on the web, improve grammar and vocabulary in the documentation and mark this PR as ready for review.

@optozorax optozorax changed the title Arbitrary data in Memory using Any Arbitrary data in Memory using Any, fix #255 Mar 28, 2021
@optozorax optozorax marked this pull request as ready for review March 28, 2021 06:00
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Excellent work!

I left some comments, but beside that I think it would also be good with a couple of unit tests, especially covering serialization/deserialization of AnyMapId. For instance, does the AnyMapId handle serializing struct State { foo: i32 } and deserializing to stuct State { foo: i32, #[serde(default)] bar: f64 } (simulating adding a new field to an existing state)?

I also think you can make your own crate out of all this if you would like! I think it could become a popular alternative to e.g. anymap and std::any::Any for others who needs serialization!

egui/src/any/mod.rs Outdated Show resolved Hide resolved
egui/src/any/mod.rs Outdated Show resolved Hide resolved
egui/src/any/any_map.rs Outdated Show resolved Hide resolved
egui/src/any/any_map.rs Outdated Show resolved Hide resolved
egui/src/any/serializable/any_map.rs Outdated Show resolved Hide resolved
egui/src/any/serializable/any_map.rs Outdated Show resolved Hide resolved
egui/src/any/serializable/element.rs Outdated Show resolved Hide resolved
egui/src/any/serializable/element.rs Outdated Show resolved Hide resolved
egui/src/any/element.rs Show resolved Hide resolved
@optozorax
Copy link
Contributor Author

Okay, thank you! Right now I am realized that I have no more time, so I can continue this PR after 6 April.

Also, thanks about the idea of moving this out as a crate 🤔

optozorax and others added 15 commits April 9, 2021 22:36
* rename `DataElement` -> `AnyMapElement`
* make `data` in `Memory` as public field of type with public interface
* make interface more rich
* transform most unwraps to proper error handling
* make `AnyMap` store by `TypeId`, so now individual type can be counted and reset
* replace `serde_json` -> `ron`
* move `any_map` to module
* separate `AnyMap` into `AnyMapId` and `serializable::AnyMapId` in order to not require `serde` traits in methods
* add `AnyMap` and `serializable::AnyMap` that stores elements just by type, without `Id`
* write documentation
* change tooltips and color picker to use `Memory::data_temp`
* rename `reset` → `remove`
* add function `remove_by_type`
* format code
@optozorax
Copy link
Contributor Author

@emilk everything is done.

@optozorax
Copy link
Contributor Author

I don't like all this copy-pasting in any_map and type_map files. So, I think it's possible to remove this copy-pasting by using type syntax and generics. I should try this.

@optozorax
Copy link
Contributor Author

optozorax commented Apr 10, 2021

No, I believe this is not possible, because this may require an associated trait inside of a trait to generically handle AnyMapElement and serializable::AnyMapElement.

* make identical interface for serialized and simple maps
* make serialized maps serialized fully, without features by moving this into `Memory` struct with `#[cfg(feature = "persistence")]` under fields
* move `serialized::TypeId` into `AnyMapElement` struct
@optozorax
Copy link
Contributor Author

However, copy-pasting between TypeMap/serializable::TypeMap and between AnyMap/serializable::AnyMap can be eliminated by using incude!() macro, but this is kinda ugly. I could do this, if you find this more proper than copy-pasting.

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Amazing work - thank you! ⭐


# Only needed if you plan to use the same egui::Context from multiple threads.
single_threaded = ["epaint/single_threaded"]
multi_threaded = ["epaint/multi_threaded"]

[dev-dependencies]
serde_json = "1"
Copy link
Owner

Choose a reason for hiding this comment

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

we could use ron for the tests too

};

let mut map: TypeMap = serde_json::from_str(&file_string).unwrap();
assert!(map.get::<StateNew>().is_none());
Copy link
Owner

Choose a reason for hiding this comment

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

Oh right, because TypeId changes. In the future it would be nice to not rely on TypeId, but perhaps use a per-type UUID or similar to handle it. That would make persistence work across rustc versions too.


/// Stores any object by `Key`.
#[derive(Clone, Debug)]
pub struct AnyMap<Key: Hash + Eq>(HashMap<Key, AnyMapElement>);
Copy link
Owner

Choose a reason for hiding this comment

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

Now that this no longer relies on Id it would be trivial to move to its own crate - hint, hint, nudge, nudge ;)

@emilk emilk merged commit 186362a into emilk:master Apr 12, 2021
@optozorax optozorax deleted the memory-arbitrary-data branch April 13, 2021 01:51
@emilk emilk mentioned this pull request Oct 24, 2021
emilk added a commit that referenced this pull request Oct 27, 2021
This refactors the widget state storage introduced by @optozorax in #257

* Unify the four buckets (`data`, `data_temp`, `id_data` and `id_data_temp`) into a single `data`.
  * Less complexity, and also less chance of error (storing in one bucket, reading from another).
* Store data by `Id` and `TypeId`.
  * Users can thus reuse the same `Id` to store many types.
  * Uses a simple xor of id and typeid, which is fast and good since both id and typeid are already high-entropy hashes.
* Use different suffixes on the functions to pick if you want the data persisted or not (`get_temp`, `insert_persisted`, etc).
  * Writing with one suffix and reading with the other works.
* To store state not bound to a specific `Id` (i.e. only based on type), use the new `Id::null` as the key.
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.

None yet

3 participants