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

Move bevy_math's implementations of Reflect into bevy_math #13456

Closed
mweatherley opened this issue May 21, 2024 · 1 comment · Fixed by #13520
Closed

Move bevy_math's implementations of Reflect into bevy_math #13456

mweatherley opened this issue May 21, 2024 · 1 comment · Fixed by #13520
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Reflection Runtime information about types C-Enhancement A new feature D-Straightforward Simple bug fixes and API improvements, docs, test and examples

Comments

@mweatherley
Copy link
Contributor

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

Right now, bevy_math's implementations of Reflect all live in bevy_reflect behind an optional feature flag. The problem with this is that it relies on the fact that all of the reflected types so far have no private fields. Private fields are inaccessible from bevy_reflect, so this organizational choice places limitations indirectly on data in bevy_math.

I ran into this limitation while working on curve-based animation.

What solution would you like?

bevy_math should provide its own Reflect implementations behind an optional dependency on bevy_reflect. This makes more sense practically and organizationally, since bevy_math actually owns those types and has access to their private data.

What alternative(s) have you considered?

Make all fields of all bevy_math types with Reflect public forever.

@mweatherley mweatherley added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels May 21, 2024
@MrGVSV MrGVSV added A-Reflection Runtime information about types A-Math Fundamental domain-agnostic mathematical operations D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed S-Needs-Triage This issue needs to be labelled labels May 21, 2024
@Olle-Lukowski
Copy link
Contributor

I will get started on moving these implementations.

github-merge-queue bot pushed a commit that referenced this issue May 27, 2024
# Objective

Fixes #13456 

## Solution

Moved `bevy_math`'s `Reflect` impls from `bevy_reflect` to `bevy_math`.


### Quick note
I accidentally used the same commit message while resolving a merge
conflict (first time I had to resolve a conflict). Sorry about that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Reflection Runtime information about types C-Enhancement A new feature D-Straightforward Simple bug fixes and API improvements, docs, test and examples
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants