added basic serialization #4

PhaestusFox · 2021-06-01T11:16:56Z

added a feature to the create called "serialize" that adds the ability to save and load the actions of an input map to a file. also made an example of how one might use said feature and a bunch of tests to make sure what is save and what is loaded is the same.

could maybe make it auto load files if the exist but this would need some thought about how to expose the save path to the developer.

could also add a version/merge system so that if an old keybind file is loaded new key binds will be added automatically; didn't do the auto merging here because it would mean working out a way to not override the custom keybinds and also not conflict with them

added a feature to the create called "serialize" that adds the ability to save and load the actions of an input map to a file. also made an example of how one might use said feature and a bunch of tests to make sure what is save and what is loaded is the same. could maybe make it auto load files if the exist but this would need some thought about how to expose the save path to the developer. could also add a version/merge system so that if an old keybind file is loaded new key binds will be added automatically; didn't do the auto merging here because it would mean working out a way to not override the custom keybinds and also not conflict with them

Cargo.toml

examples/hello_world_serde.rs

src/serialize.rs

ndarilek · 2021-06-01T15:33:58Z

src/serialize.rs

+        let ron_string = std::fs::read_to_string(path)?;
+        let actions = ron::from_str(&ron_string).expect("Failed to get actions from ron string");
+        self.actions = actions;
+        //may need to clear self here but i dont really know what that does


I think you're good but I'll validate. IIRC clear only clears the active state of keybindings to work around bevyengine/bevy#1700.

ndarilek · 2021-06-01T15:34:39Z

src/serialize.rs

+        //may need to clear self here but i dont really know what that does
+        Ok(())
+    }
+    pub fn new_from_path(path : &str) -> std::io::Result<InputMap<T>> where T : DeserializeOwned{


Would it make sense to use actual Paths here?

It should be new_from_path<T: Into<Path>>(path T) but I don't know how to do that, because when I tried it told me Path is a private type; maybe I was just getting the wrong Path Type.

ndarilek · 2021-06-01T15:35:30Z

Looks great. Thanks for taking this on!

ndarilek · 2021-06-01T18:44:53Z

Actually, having thought about this a bit more, here's what I think I'd like to see:

Add a method, something like pub fn set_bindings(actions: HashMap<T, Action>) for directly setting actions from a HashMap.
Make ron a dev-dependency, move the serioalization/deserialization code to the example, and remove the existing serialization mod.

My concern is that this change a) serializes keybindings as their own thing and b) specifically uses ron. In my game's case, I want input bindings to be a key on my existing settings object, which already supports serialization to a non-RON format. So I'd end up not using the file serialization code at all, which will probably be game-specific depending on how someone's storing settings anyway.

So, IOW, we a) keep the existing serialization feature and b) move the current RON file serialization/deserialization support to the example. Nothing is lost, we're just reshuffling where the code goes. Does that work for you?

* added dependencies versions to .toml * made ron a dev-dependencie * changed keybinds -> keybindings *made example setup.system() one system with compile time branch * made serialize module purely for serialize feature tests * added test.keymap to .gitignore * made InputMap test one generic test that then tests String and a custom enum ? still unclear if I would need to clear the InputMap after changing the action to pickup any potential new actives/ remove actives that are no longer so *added methods to get/set/"add a group of" actions to InputMap this should allow for user impl for serialisation/de-serialisation and impl them in the example

PhaestusFox · 2021-06-02T09:01:59Z

I put some comments in the code about this but I'm also going to make this post asking more directly.
would you consider adding a copy of the T type that represents the action taken to the Action struct and replacing the Action
struct as it stands currently ether with just a raw Vec or a new struct Bindings.
something like this

struct Action<T>{
 action: T,
 bindings: Bindings,
}

struct Bindings{
bindings: Vec<Binding>
}

impl `"current Action impl"` for Bindings

impl `bind(binding : Into<Binding>)` for Action/Bindings

this would allow for a more readable public API in my opinion

a user would be able to bind multiple bindings to one action without having to declare the action over and over as they do now
can leave current fn bind so you can still declare the action and bind to it in one call
my serialisation functions can use a Vec<Action> instead of Vec<T, Action> being more clear
makes the get_actions more clear returning &HashMap<T, Bindings> over &HashMap<T, Action>; T is the action for the user
makes get_action more clear returning Some(&Bindings) forgiven T, over Some(&Action) forgiven T; T is the action for the user
I think I could make some nice clean merge code for the actions hashmap so defaults don't override preexisting changes
? could also maybe use this for binding conflict detection but don't know if you would really need/want that by default
? could also maybe use it to have nicer control over if a keybinding triggers retroactively with modifiers. I made an issue about this

ndarilek · 2021-12-14T15:38:57Z

I'm (finally!) looking into merging this, but I've since relicensed to MIT + Apache 2 in order to be more compatible with Bevy. Are you OK with this license change?

Thanks!

PhaestusFox · 2021-12-22T06:33:45Z

yeah I'm all good with that