bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.04k stars 3.44k forks source link

Re-export SmolStr from winit #12448

Open viridia opened 6 months ago

viridia commented 6 months ago

What problem does this solve or what need does it fill?

Bevy's character input events represent the text as a SmolStr, which is a type defined in winit. (This used to be a char but it changed in 13.0). Unfortunately, this type is not re-exported by Bevy, so anyone who wants to store a typed character has to have a direct dependency on winit. This is less than desirable for a number of reasons; for example, a future version of Bevy might use a different window manager.

What solution would you like?

Have Bevy re-export the SmolStr type from winit.

What alternative(s) have you considered?

An alternative would be for Bevy to define it's own SmallStr type. This would provide even better isolation from the vagaries of winit.

Additional context

Note that with the recent change, I can no longer call text.is_control() to detect whether the typed key is a control char.

tomara-x commented 6 months ago

re: is_control, you can get the char out of the SmolStr and call is_control on it normally

if let Some(c) = text.chars().nth(0) {
    if c.is_control() {}
}

i think it's just a str so you can treat it like one

viridia commented 6 months ago

All right, that's helpful.

SolarLiner commented 5 months ago

So from the issue and above discussion, there are several directions for this:

  1. Keep the API as-is, SmolStr from winit has impl Deref<Target=str> so it's usable regardless
  2. Make the APIs return &str directly, it hides the SmolStr as implementation detail and makes re-exporting it unneeded. This has the issue that we are going from a owned type to a reference type, which means having to deal with lifetimes
  3. Re-export SmolStr. This makes explicit that the type in the API is intended, but direct re-export exposes us to unexpected API changes from winit
  4. Have our own small string implementation, convert from winit's, and expose that to the user. It's more boilerplate for internal Bevy code but should not make user code any different.
  5. (provided for extensiveness) revert the winit API change and expose a single char again. I don't like this one because the change in winit feels intentional, and so means that we would probably be discarding information in the cases where the winit-provided string is longer than a single character.

From the above I would personally go for either 1. or 4.

viridia commented 5 months ago

The specific case I am concerned about is the ReceivedCharacter event in Bevy. This returns a SmolStr rather than a character because a typed key can produce multiple unicode characters. That being said, the docs for SmolStr say that it's 23 bytes long, and intended to be "good enough default storage for tokens of typical programming languages". 23 bytes might be overkill for a typed char, but I don't suppose that really matters, it's not like the user is typing thousands of keys per second.

My main issue is that users who are concerned with handling keystrokes should not need to be aware of the underlying window system.

One solution would be to provide an accessor on ReceivedCharacter that returns a &str. This would allow users to ignore the underlying type in the event, while avoiding lifetime issues around passing a reference inside an event.