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.

Sign up for GitHub

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

Jump to bottom

bevy_reflect: Adding support for Atomic values #14419

Merged
merged 6 commits into from
Jul 29, 2024
Merged

bevy_reflect: Adding support for Atomic values #14419

merged 6 commits into from
Jul 29, 2024

Conversation

recatek
Copy link
Contributor

@recatek recatek commented Jul 21, 2024

Fixes #14418

Note that this does not add AtomicPtr, which would need its own special casing support, just the regular value types.
Also, I was forced to be opinionated about which Ordering to use, so I chose SeqCst as the strictest by default.

@github-actions GitHub Actions
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 ✨

@recatek
Copy link
Contributor Author

recatek commented Jul 21, 2024 •
edited
Loading

Looks like the build is failing due to an unrelated warning in bevy_render.

@BD103 BD103 added A-Rendering Drawing game state to the screen A-Reflection Runtime information about types C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Domain-Expert Requires deep knowledge in a given domain S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Macros Code that generates Rust code and removed A-Rendering Drawing game state to the screen labels Jul 22, 2024
@BD103 BD103 requested a review from MrGVSV July 22, 2024 02:28
MrGVSV
MrGVSV reviewed Jul 22, 2024
View reviewed changes
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.

I think SeqCst is the right choice here, but I'm not familiar enough with atomics to fully make that decision. If someone else has feedback there, please feel free to leave a comment!

Otherwise just a couple suggestions!

crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/impls/std.rs Outdated Show resolved Hide resolved
@recatek
- Adding ReflectSerialize and ReflectDeserialize
ce9ef66 
- Moving impl_type_path! to the top of the macro and adding impl_function_traits!
- Removing fully qualified type paths in the macro (but adding them for the Ordering type for consistency)
@recatek
Merge branch 'bevyengine:main' into main
56469f9
@vacuus
Copy link

vacuus commented Jul 22, 2024

Relaxed ordering should be fine. Those methods don't seem like points for synchronization, especially because they would need to synchronize with a store somewhere.

@recatek
Copy link
Contributor Author

recatek commented Jul 22, 2024

I think I've addressed all of the feedback. Regarding Relaxed vs. SeqCst, SeqCst is the strictest option and is the default in other languages when you don't specify one. The only downside is pretty minor perf hits, and even then primarily on ARM rather than x86 (which has strong memory guarantees to begin with). Since this is in reflection code to begin with and because the user won't be able to control the ordering mode, I don't think the perf hit is as bad as potentially unexpected behavior.

@recatek
Copy link
Contributor Author

recatek commented Jul 23, 2024

Is this still S-Needs-Review? I'm new to bevy PRs so not sure how all the labels work.

@MrGVSV
Copy link
Member

MrGVSV commented Jul 23, 2024

Is this still S-Needs-Review? I'm new to bevy PRs so not sure how all the labels work.

Yeah you need 2 community approvals before getting moved to S-Ready-For-Final-Review :)

@MrGVSV MrGVSV added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 23, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 29, 2024
Merged via the queue into bevyengine:main with commit 87b63af Jul 29, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MrGVSV MrGVSV MrGVSV approved these changes

@vacuus vacuus vacuus approved these changes

Assignees
No one assigned
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible D-Domain-Expert Requires deep knowledge in a given domain D-Macros Code that generates Rust code D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reflect: Support for Atomic values
5 participants
@recatek @vacuus @MrGVSV @alice-i-cecile @BD103