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

Hide fixed_update behind a feature flag #14911

Closed
wants to merge 14 commits into from
Closed

Hide fixed_update behind a feature flag #14911

wants to merge 14 commits into from

Conversation

Lubba-64
Copy link
Contributor

@Lubba-64 Lubba-64 commented Aug 25, 2024 •
edited
Loading

Objective

Solution

  • Move all relevant code behind cfg directives.

Testing

  • Working through compile bugs in this PR should be satisfactory.

@Lubba-64 Lubba-64 changed the title feat: Hide fixed-update behind a feature flag Hide fixed-update behind a feature flag Aug 25, 2024
@Lubba-64
Copy link
Contributor Author

Apparently bevy_gizmos only uses fixed_update scheduling...

@Lubba-64
Copy link
Contributor Author

Apparently bevy_gizmos only uses fixed_update scheduling...

Whoever sees this first let me know why this is even a thing and whether we should change it.

@Lubba-64 Lubba-64 changed the title Hide fixed-update behind a feature flag Hide fixed_update behind a feature flag Aug 25, 2024
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-App Bevy apps and plugins A-Time Involves time keeping and reporting labels Aug 25, 2024
@theprotonfactor
Copy link

theprotonfactor commented Aug 25, 2024 •
edited
Loading

Based on the migration guide, it appears as though this feature will be disabled by default. Is that correct? The attached issue mentions that FixedUpdate should be enabled by default. I'll strongly advocate for that stance as well. It's basically a requirement for any game that needs a physics engine and at least for 3D games, that's the norm rather than the exception. It's also a common enough staple in other engines that I think making users opt out rather than opt in is the better choice here.

@alice-i-cecile
Copy link
Member

The gizmo stuff is from #10973 by @Aceeri. With the feature flag off, we should reproduce the previous behavior there.

@alice-i-cecile alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label Aug 25, 2024
@Lubba-64
Copy link
Contributor Author

The gizmo stuff is from #10973 by @Aceeri. With the feature flag off, we should reproduce the previous behavior there.

based on the fact that this seems very intentional, my proposed solution will be to have different behavior based on whether that flag is disabled.

@Lubba-64
Copy link
Contributor Author

I ran some examples and it seemed like the gizmos behavior was preserved

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-Gizmos Visual editor and debug gizmos C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Aug 25, 2024
mnmaita
mnmaita reviewed Aug 25, 2024
View reviewed changes
crates/bevy_time/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_time/src/lib.rs Show resolved Hide resolved
crates/bevy_app/src/lib.rs Outdated Show resolved Hide resolved
alice-i-cecile
alice-i-cecile requested changes Aug 27, 2024
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile 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! One more thing to do: add a top-level feature here, which enables a corresponding feature in bevy_internal. Otherwise it'll be quite annoying to enable this when relying on bevy alone.

@janhohenheim janhohenheim added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 5, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 5, 2024

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@github-actions GitHub Actions
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@Lubba-64
Copy link
Contributor Author

nevermind... fixed it i think.

alice-i-cecile
alice-i-cecile reviewed Sep 12, 2024
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Almost there; feature flagging looks good but I have a nit about the gizmos code.

@Lubba-64
Copy link
Contributor Author

I think that should be good let me know if I missed something.

janhohenheim
janhohenheim approved these changes Sep 13, 2024
View reviewed changes
docs/cargo_features.md
@@ -31,6 +31,7 @@ The default feature set enables most of the expected features of a game engine,
|bevy_ui|A custom ECS-driven UI framework|
|bevy_winit|winit window and input backend|
|default_font|Include a default font, containing only ASCII characters, at the cost of a 20kB binary size increase|
|fixed_time|Provides the fixed update schedules|
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should link somewhere on our docs so that new users encountering this flag don't get confused what any of that means? Could also link to the fixed timestep example, that one has a nice documentation of the feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we just include the link in the description?

@janhohenheim janhohenheim 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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 13, 2024
@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR S-Needs-SME Decision or review from an SME is required and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Sep 13, 2024
@Lubba-64
Copy link
Contributor Author

Really fun to get into the weeds with this PR. feels like I'm actually getting somewhere with my contributions to bevy.

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through and removed X-Controversial There is active debate or serious implications around merging this PR S-Needs-SME Decision or review from an SME is required labels Sep 23, 2024
@alice-i-cecile
Copy link
Member

@Jondolf @JMS55 @maniwani, if y'all have time I would appreciate your opinions on how / if we should proceed on this. I don't feel super strongly about this. The technical implementation is good, and I think this it's reasonable for some games to just not use fixed update, but I don't really care how they achieve that goal. Even a lint would be fine.

@JMS55
Copy link
Contributor

JMS55 commented Sep 24, 2024 •
edited
Loading

Haven't reviewed the code, but my opinion hasn't changed. Feature flag's make things harder to program, and especially test (we already have way too many combinations of flags).

If the goal is just to skip the schedule when unseeded to save performance, I'd rather look into ways to make empty schedules cheaper, or put it behind a runtime config in a resource or app setting or something.

I'd prefer to save feature flags for truly necessary cases, like heavy optional dependencies.

@alice-i-cecile
Copy link
Member

My larger goal here is project-level consistency: making it impossible to screw up and put some of your logic in FixedUpdate and some in Update. I think I'll swap this to a lint. Closing.

@alice-i-cecile alice-i-cecile mentioned this pull request Sep 24, 2024
Open
@Lubba-64 Lubba-64 deleted the fixed-update-cfg branch September 24, 2024 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnmaita mnmaita mnmaita left review comments

@janhohenheim janhohenheim janhohenheim approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@JMS55 JMS55 Awaiting requested review from JMS55

@Jondolf Jondolf Awaiting requested review from Jondolf

@maniwani maniwani Awaiting requested review from maniwani

Assignees
No one assigned
Labels
A-App Bevy apps and plugins A-Gizmos Visual editor and debug gizmos A-Time Involves time keeping and reporting C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Move FixedUpdate / fixed timestep behind a feature flag
6 participants
@Lubba-64 @theprotonfactor @alice-i-cecile @JMS55 @janhohenheim @mnmaita