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

Add Reflect and FromReflect to UserInput #331

Closed

Conversation

hammypants
Copy link

Related to the contributing guidelines: I note that the dev branch is very stale and I couldn't find any recent PRs against it, so I made this against main. I can adjust this if necessary.

General

This PR adds Reflect and FromReflect to UserInput via adding it to the entire chain of types under its variants that also needed them. It was mentioned in a few places that this was desired, notably in #308, and I also would like to see this. (It kinda feels like any type that is intended to be a bevy component should have them by default and not having them be opt-out, but I digress.)

PetitSet ...

The big hang up here was that PetitSet does not implement either of said traits. I naively newtype-d it and delegated a majority of its features, but I would imagine there could be a need to have more careful thought put into this. It currently is just a poor veneer over PetitSet

PetitSet is currently exposed as API surface in a couple of places; so, this is kind of a big change. I can imagine a world where its not, though, and that the functionality of an input chord is much more pared down, or perhaps looks entirely different than a front for PetitSet, and this would perhaps force that thought into the light earlier than expected.

A perhaps less obtrusive approach would be to only do this behind the scenes and maintain PetitSet as the forward API. You all own both crates so I defer all decisions!

Last thing on the newtype: it's added to a separate module, but reexported through user_input-- this was entirely a "idk lmao" decision.

@alice-i-cecile alice-i-cecile added the usability Reduce user friction label Mar 26, 2023
@alice-i-cecile
Copy link
Contributor

Definitely in favor of having these traits implemented! I'm actually planning on dropping petitset as a dependency, in favor of a simple hash or btree map in #321.

As a result, I'm a bit reluctant to merge this in the current form, but I'm open to suggestions on the best way to get to where we're hoping to be.

@hammypants
Copy link
Author

hammypants commented Mar 27, 2023

Hi @alice-i-cecile ! Thanks for looking.

I think the same primary issue arises regardless of what form PetitSet's replacement takes due to the orphan rule, unless it's a type owned by this crate (newtype included) or one of the blessed types in https://github.com/bevyengine/bevy/tree/main/crates/bevy_reflect/src/impls, no?

In the suggested link, if bevy_reflect 's traits can be derived for tuples of things that implement said traits that would be exciting from a usable user API POV.

Would it be better to walk back the chord implementation here and just grab the easy derives on everything else for now?

@alice-i-cecile
Copy link
Contributor

Good analysis: I was thinking of using one of the blessed types here :)

If you're interested in moving this forward faster, I'd be happy to merge a PR migrating to hash maps, and then we can rebase and merge this?

@hammypants
Copy link
Author

Good analysis: I was thinking of using one of the blessed types here :)

If you're interested in moving this forward faster, I'd be happy to merge a PR migrating to hash maps, and then we can rebase and merge this?

I can do that!

alice-i-cecile added a commit that referenced this pull request Oct 26, 2023
@alice-i-cecile
Copy link
Contributor

Fixing as part of #400.

@hammypants
Copy link
Author

Fixing as part of #400.

My apologies, I was hoping #321 would have landed as [at the time] removing PetitSet was looking more involved than I had time for and this seemed close.

@hammypants hammypants deleted the userinput-reflect branch October 27, 2023 20:32
@alice-i-cecile
Copy link
Contributor

Yep, not a problem at all :) That PR has been really tough.

alice-i-cecile added a commit that referenced this pull request Nov 11, 2023
* Fix typo

* Update release notes

* Straightforward migration

* Remove useless test for from impl

* Just use a Vec for chords :(

* Clone because our API is jank

* Hash all the types

* Repair tests after swapping to a Vec

* Simple fixes for binding_map

* Remove binding_menu egui example due to maintenance burden

* Clippy

* Fix idempotency of insertion

* Remove brittle serde test

* Simple doc test fixes

* Remove outdated references to 16 max input maps

* Implement Reflect for InputMap

Closes #386.

* Reflect UserInput

Closes #331

* Simple fix for doc test

* Fix mismatched HashMap types in doc test

* Remove example using removed API

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

* 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>

---------

Co-authored-by: Tomato <67799071+100-TomatoJuice@users.noreply.github.com>
Co-authored-by: Alice Cecile <alice.i.cecil@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
usability Reduce user friction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants