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

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

Hide fixed_update behind a feature flag #14911

wants to merge 14 commits into from
+130 −51

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 simple 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 simple 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 simple 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 C-Breaking-Change 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.

@alice-i-cecile alice-i-cecile 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 Sep 8, 2024
@Lubba-64
Copy link
Contributor Author

whoops should have waited for the pipelines to finish, should be good though.

@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.

@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
janhohenheim
janhohenheim reviewed Sep 13, 2024
View reviewed changes
crates/bevy_gizmos/Cargo.toml
webgl = []
webgpu = []
fixed_time = ["bevy_app/fixed_time"]
Copy link
Member

@janhohenheim janhohenheim Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

maybe there should be a comment here or in the lib.rs documentation that this crate / plugin doesn't do much without a fixed timestep? Otherwise I can already see people in Discord asking why their gizmos are not working in their minimal setup.

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janhohenheim janhohenheim janhohenheim approved these changes

@mnmaita mnmaita mnmaita left review comments

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

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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use S-Needs-SME Decision or review from an SME is required 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.

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