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

Make timestep mutable #2637

Closed
wants to merge 1 commit into from
Closed

Make timestep mutable #2637

wants to merge 1 commit into from

Conversation

hanabi1224
Copy link

@hanabi1224 hanabi1224 commented Aug 11, 2021 •
edited
Loading

Objective

Solution

  • Move timestep state to behind a smart pointer

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Aug 11, 2021
@DJMcNab
Copy link
Member

DJMcNab commented Aug 11, 2021

I this is a fine change by itself, but it feels like there should be a solution which is more integrated into the ECS itself, somehow

Like these locks should not be needed, and instead the mutability 'locking' could be managed by the scheduler.

hanabi1224
hanabi1224 commented Aug 11, 2021
View reviewed changes
crates/bevy_core/src/time/fixed_timestep.rs
@@ -83,24 +95,14 @@ impl FixedTimestep {
}
}

pub fn with_label(mut self, label: &str) -> Self {
self.state.label = Some(label.to_string());
pub fn with_label(mut self, label: impl Into<String>) -> Self {
Copy link
Author

@hanabi1224 hanabi1224 Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

this impl Into<String> change is irrelevant, it saves an extra string clone when label String is created on the fly and user just want to hand over ownership, which is not allowed with a &str parameter

@hanabi1224 hanabi1224 changed the title [Draft ]Make timestep mutable Make timestep mutable Aug 12, 2021
@alice-i-cecile alice-i-cecile added A-Core C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed S-Needs-Triage This issue needs to be labelled labels Aug 16, 2021
@alice-i-cecile
Copy link
Member

@maniwani, what do you want to do with this one?

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR S-Needs-Design This issue requires design work to think about how it would best be accomplished and removed S-Needs-Design This issue requires design work to think about how it would best be accomplished labels May 3, 2022
@alice-i-cecile
Copy link
Member

I think we can tackle this issue in #5752 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Design This issue requires design work to think about how it would best be accomplished X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hanabi1224 @DJMcNab @alice-i-cecile