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

bevy_reflect: implement Reflect for SmolStr #8771

Merged
merged 9 commits into from Jun 8, 2023

Conversation

Vrixyz
Copy link
Member

@Vrixyz Vrixyz commented Jun 6, 2023

Objective

To upgrade winit's dependency, it's useful to reuse SmolStr, which replaces/improves the too restrictive Key letter enums.

As Input is a resource it should implement Reflect through all its fields.

Solution

Add smol_str to bevy_reflect supported types, behind a feature flag.

This PR blocks winit's upgrade PR: #8745.

Current state

  • I'm discovering bevy_reflect, I appreciate all feedbacks, and send me your nitpicks!
  • Lacking more tests

@Vrixyz Vrixyz mentioned this pull request Jun 6, 2023
11 tasks
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Looking good! Just a few comments.

crates/bevy_reflect/Cargo.toml Outdated Show resolved Hide resolved
crates/bevy_reflect/Cargo.toml Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/smol_str.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/smol_str.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/smol_str.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/smol_str.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/smol_str.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/smol_str.rs Show resolved Hide resolved
@MrGVSV MrGVSV added A-Windowing Platform-agnostic interface layer to run your app in C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Jun 6, 2023
Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Jun 7, 2023
@alice-i-cecile
Copy link
Member

This is a good idea, and the use of a feature flag is wise. Generally looks good, although I agree with the nits raised :)

Vrixyz and others added 3 commits June 7, 2023 09:37
Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
crates/bevy_reflect/src/impls/smol_str.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/smol_str.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/smol_str.rs Outdated Show resolved Hide resolved
Vrixyz and others added 3 commits June 7, 2023 23:27
Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>
@Vrixyz
Copy link
Member Author

Vrixyz commented Jun 8, 2023

This is a good idea, and the use of a feature flag is wise. Generally looks good, although I agree with the nits raised :)

I thought too, but as current implementation of #8745, I changed the exposed character key to contain a SmolStr, effectively adding a dependency to the bevy_input.

So I aggregated the features within bevy rather than fragmenting too much the features: 012bd90. (Now that I think about it, maybe we shouldn't add this into the bevy feature just yet, but include it in the winit PR?)

We could also add more granular features but I think that's not the goal of this PR.

I'm setting this out of draft mode.

@Vrixyz Vrixyz marked this pull request as ready for review June 8, 2023 07:44
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Now that I think about it, maybe we shouldn't add this into the bevy feature just yet, but include it in the winit PR?

Yeah the changes are small enough that you probably could. Approving this PR anyways in case it makes more sense to merge beforehand.

We could also add more granular features but I think that's not the goal of this PR.

Yeah I think it would be good to have a feature for any bevy-crate dependency (i.e. a bevy_math feature to add glam, etc.). But yeah we can focus on that another time.

@alice-i-cecile
Copy link
Member

This is useful in its own right, and much easier to review like this than as part of the mega upgrade PR. Merging now.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 8, 2023
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 8, 2023
Merged via the queue into bevyengine:main with commit b559e9b Jun 8, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types A-Windowing Platform-agnostic interface layer to run your app in C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants