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

Feature flag to add repr(c) to bevy types #14362

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ScottKane
Copy link

@ScottKane ScottKane commented Jul 17, 2024

Objective

To add repr(C) to bevy types that could/should be able to be sent over FFI behind a feature flag.

Solution

Added repr_c feature which propagates to relevant crates to conditionally add #[repr(C)] to bevy built in component/math/bundle types.

Testing

I have run through the test suite with everything working as expected, I have also created test bevy apps to compare struct layouts being as expected and working correctly. I have also tested with my C# bindings to bevy (the original motivation behind this PR) to ensure the struct layout for all types is correct when sent over FFI.

This has only been tested on Fedora x86 64.


Migration Guide

There aren't any breaking changes as such however there is a few minor changes in some function calls that depend on glam::Quat, the reason for this is that #[repr(simd)] types currently aren't compatible with FFI (see rust-lang/rust#53346) so I had to create a wrapper type that can switch between the glam version and a repr(C) compatible Quat. As a result of this, the glam types we are using have functions that expect a glam::Quat and not the new wrapper. I have moved these specific functions into the wrapper for now but I think the best option would likely be to have wrappers for all of the glam types we use so the usage would stay exactly the same.

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@ScottKane
Copy link
Author

There is still some work to get bundle types working over FFI but this initial work allows the passing of core types like Transform and GlobalTransform. Most of the types from glam are already compatible with glam::Quat being the exception.

@mweatherley mweatherley added C-Feature A new feature, making something new possible A-Transform Translations, rotations and scales A-Math Fundamental domain-agnostic mathematical operations X-Controversial There is active debate or serious implications around merging this PR labels Jul 17, 2024
@janhohenheim janhohenheim added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label Jul 17, 2024
@janhohenheim
Copy link
Member

janhohenheim commented Jul 17, 2024

I can't really comment on the validity of this, but I want to mention that this PR looks prone to suffering from merge conflicts due to the amount of code touched, so it would be nice to get it sorted quickly.

@ScottKane I notice that there is also a lot of tiny refactoring in this PR. While that is generally appreciated, I think it will make life harder for you in this case, as it makes an already large and controversial PR harder to review and increases the chance that you run into merge conflicts.

@BD103
Copy link
Member

BD103 commented Jul 17, 2024

To add repr(C) to bevy types that could/should be able to be sent over FFI behind a feature flag.

Do you have a specific use-case for #[repr(C)] here? Bevy is a Rust-first engine, so support for other languages is not a high priority. I feel like you'll need a good reason to introduce this code complexity, since I'm not sure how useful it will be outside of a few niche areas.

For instance, I could see how this would be useful when working with dynamic libraries, but actual C FFI is far less compelling. (And even then, the amount of generics we use limits dynamic libraries significantly.)

@mweatherley
Copy link
Contributor

I would be much more inclined towards moving forward with this if the needed changes involving quaternions or whatever were made upstream in glam. I really don't think maintaining a duplicate version of upstream code just to circumvent glam is a good decision in terms of the maintenance of bevy_math.

@ScottKane
Copy link
Author

@janhohenheim the minor changes are there to support calling functions using the Quat wrapper, the alternative would be the user having to call .into() everywhere which felt like a bad idea.

@BD103 I am currently writing C# bindings for bevy using the new dynamic API's (SystemBuilder/QueryBuilder) and this work allows bevy built-ins to be shared with the C# implementation, the alternative would be that anyone wanting to bind to bevy would have to duplicate all of these types into their native code to allow it to pass over FFI. For the most part ignoring the Quat changes all the PR does is add a feature flag to define a struct layout with minimal impact.

@mweatherley I agree, I could open a PR in glam with this Quat code which would massively reduce the complexity of maintaining this. If I can get that PR accepted into glam I will amend this PR to update glam version and only add conditional repr(C)'s where needed.

@BD103 BD103 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 17, 2024
@BD103
Copy link
Member

BD103 commented Jul 17, 2024

@BD103 I am currently writing C# bindings for bevy using the new dynamic API's (SystemBuilder/QueryBuilder) and this work allows bevy built-ins to be shared with the C# implementation, the alternative would be that anyone wanting to bind to bevy would have to duplicate all of these types into their native code to allow it to pass over FFI. For the most part ignoring the Quat changes all the PR does is add a feature flag to define a struct layout with minimal impact.

Ok. That reasoning makes sense, though I think a maintainer should probably weigh in here as well. I'm going to mark this as "waiting for author" until the glam changes resolve. :)

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-Transform Translations, rotations and scales C-Feature A new feature, making something new possible S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants