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

Remove petitset dependency #400

Merged
merged 22 commits into from
Nov 11, 2023
Merged

Remove petitset dependency #400

merged 22 commits into from
Nov 11, 2023

Conversation

alice-i-cecile
Copy link
Contributor

@alice-i-cecile alice-i-cecile commented Oct 17, 2023

While this set / map crate is well-suited to our needs in several ways, it's not ideal as:

  1. The vector based approach in InputMap is hard to translate to a trait-based approach (UserInput as trait #346), or extend to nested or dynamic actions.
  2. The UserInput's memory footprint is hilariously large, as it must store an array-like type.
  3. petitset is a nonstandard (if small) dependency.

This PR explores replacing it completely, using HashMaps (currently a MultiMap).

There are several drawbacks to this however:

  1. We cannot store a HashSet in our Chord variant, as it is not itself hash.
  2. We cannot use a BTreeSet either, as the stored types aren't Ord.
  3. As a result, Chord has to store a simple Vec (or array), losing its uniqueness checks.
  4. It becomes quite hard to reason about "what is keybinding 2 for this action" in the classical key bindings menu way. Supporting this requires storing either a HashMap<A, Vec<Option<UserInput>> in our input maps (😩) or an explicit slot-like design with a fixed number of possible bindings.

Open Questions

  1. Do we keep petitset, and use it only for chords? This allows us to retain uniqueness guarantees but doesn't help our memory footprint.
  2. How do we solve the "input binding menu" problem?
    a) Give up, this is a dumb thing to try and model, it should be built into your menu logic not the input map.
    b) Store a map of maps
    c) Store an array of maps
    d) Store each map as a different field
    e) Abandon this PR completely

@alice-i-cecile alice-i-cecile added usability Reduce user friction performance Code go brrr breaking-change A breaking change that requires a new major version controversial Requires a heightened standard of review labels Oct 17, 2023
@sixfold-origami
Copy link
Contributor

For the input binding menu, I think it's ok to let that fire burn for now. This always struck me as more of a menu logic thing anyway, not really an input mapping problem. We may be proven wrong in the future, but I'm not too worried for the time being.

@sixfold-origami
Copy link
Contributor

I also think it's ok that chords are stored as Vecs. We should probably make sure there's a docs warning for this (namely that uniqueness it not checked), but I think it's ok.

@alice-i-cecile
Copy link
Contributor Author

Conclusion on the design questions above:

  1. We just use a Vec, and enforce uniqueness on the constructor.
  2. Choose solution a. I've cut the input binding menu example as part of this PR, and opened Add a key binding example #403 for a replacement to avoid blocking this work.

@alice-i-cecile alice-i-cecile marked this pull request as ready for review October 25, 2023 20:10
@100-TomatoJuice
Copy link
Collaborator

100-TomatoJuice commented Nov 9, 2023

Restating here for documentation. The failures are due to A::variants(), I think.

With an enum like this:

#[derive(Actionlike, PartialEq, Eq, Clone, Copy, Hash, Debug, Reflect)]
enum AxislikeTestAction {
    X,
    Y,
    XY,
}

It only checks the variant X. This happens in .which_pressed() on InputMap. The for loop that loops through A::variants() only loops once: not doing anything the other two variants. But, the examples return all variants? I'm now looking for the cause of the A::variants() only returning one.

For this enum, I added inputs for Jump and Run, but not A.

enum Action {
    Jump,
    A,
    Run,
}

The result was Jump worked but Run did not as A blocked of any lower actions for some reason.

100-TomatoJuice and others added 2 commits November 9, 2023 12:21
* Update for Bevy 0.12 (#408)

* Update to git

* forgot a configure_set

* Update examples

* Update Bevy to 0.12 release

* Bump versions

* Disable failing test

* Remove bevy_egui so we can ship

* Update release notes

---------

Co-authored-by: Tomato <67799071+100-TomatoJuice@users.noreply.github.com>
Co-authored-by: Alice Cecile <alice.i.cecil@gmail.com>

* Add back `bevy_egui` to LWIM 0.11 (#409)

* Update to `bevy_egui` 0.23 for Bevy 0.12

* Bump to 0.11.1

* Update RELEASES.md

* Switch `A::variants()` to `self.iter()`

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecil@gmail.com>
@100-TomatoJuice
Copy link
Collaborator

100-TomatoJuice commented Nov 11, 2023

There are still quite a bit of A::variants() usage left. I'm kinda surprised that it has not broken anything else like it did. Is it within the scope of this pr to remove usage of A::variants() as you mentioned on Discord? The removal of A::variants() will not be trivial, I think. For InputMap it can use iter(), but ActionState doesn't have access to the map so it will require some thought.

@alice-i-cecile alice-i-cecile marked this pull request as ready for review November 11, 2023 21:27
@alice-i-cecile
Copy link
Contributor Author

Let's pull that out into a separate PR and merge this now :)

@alice-i-cecile alice-i-cecile merged commit 6d98b8e into main Nov 11, 2023
4 checks passed
@alice-i-cecile alice-i-cecile deleted the petitset-yeet branch November 11, 2023 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change A breaking change that requires a new major version controversial Requires a heightened standard of review performance Code go brrr usability Reduce user friction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants