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

[Merged by Bors] - Change breakout to use fixed timestamp #1541

Closed
wants to merge 4 commits into from

Conversation

hymm
Copy link
Contributor

@hymm hymm commented Mar 3, 2021

This PR fixes #1240, where the ball is escaping the playing field at low framerates. I did this by moving the movement and physics system into a Fixed Timestep system set and changing the movement steps to a constant. So we lose the example use of delta_time for changing position, but gain a use of FixedTimestep.

If this is accepted bevyengine/bevy-website#102 will need to be updated to match.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples labels Mar 3, 2021
@mockersf
Copy link
Member

mockersf commented Mar 3, 2021

need a rebase to fix CI

@hymm hymm force-pushed the fix_breakerout_fixed_timestep branch from f442a65 to eaaef00 Compare March 4, 2021 06:01
@hymm hymm closed this Mar 4, 2021
@hymm hymm reopened this Mar 4, 2021
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.

This is a clean change, and fixing strange behavior and obvious bugs in our example games is worth the added complexity.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 8, 2021
examples/game/breakout.rs Outdated Show resolved Hide resolved
@hymm hymm force-pushed the fix_breakerout_fixed_timestep branch 2 times, most recently from 71fff34 to aa1542a Compare March 10, 2021 01:49
@hymm
Copy link
Contributor Author

hymm commented Mar 10, 2021

Changed the speeds to 8.0 and 6.0 as these were whole numbers close to the original speed and from my eye test it seems like whole numbers animate a little smoother.

I should mention that this PR doesn't fix everything from #1240 as there are still some wonky collision detection between the ball and the paddle.

@hymm hymm force-pushed the fix_breakerout_fixed_timestep branch from aa1542a to a46f6d6 Compare March 14, 2021 03:35
@@ -63,7 +72,7 @@ fn setup(
..Default::default()
})
.with(Ball {
velocity: 400.0 * Vec3::new(0.5, -0.5, 0.0).normalize(),
velocity: 6.0 * Vec3::new(0.5, -0.5, 0.0).normalize(),
Copy link
Member

Choose a reason for hiding this comment

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

This is still "not velocity" with the new implementation. You effectively pre-multiplied TIME_STEP and velocity. This has the effect of changing object speed if someone changes the timestep value here: https://github.com/bevyengine/bevy/pull/1541/files#diff-4dcbfd17ebad2621b5605be8bd3b43859376e27f30793a937858d470627ed2b2R18

Copy link
Member

Choose a reason for hiding this comment

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

movement speed should be timestep independent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. I shouldn't read comments right before I fall asleep. Should be fixed now.

@hymm hymm force-pushed the fix_breakerout_fixed_timestep branch from a46f6d6 to c02bf1c Compare March 15, 2021 23:45
@hymm hymm force-pushed the fix_breakerout_fixed_timestep branch from c02bf1c to 19a1863 Compare April 5, 2021 17:34
@cart
Copy link
Member

cart commented Apr 12, 2021

Just adapted this to use the newly-added SystemSet. This slightly reduces boilerplate.

@cart
Copy link
Member

cart commented Apr 12, 2021

bors r+

bors bot pushed a commit that referenced this pull request Apr 12, 2021
This PR fixes #1240, where the ball is escaping the playing field at low framerates.  I did this by moving the movement and physics system into a Fixed Timestep system set and changing the movement steps to a constant.  So we lose the example use of delta_time for changing position, but gain a use of FixedTimestep.

If this is accepted bevyengine/bevy-website#102 will need to be updated to match.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors bors bot changed the title Change breakout to use fixed timestamp [Merged by Bors] - Change breakout to use fixed timestamp Apr 12, 2021
@bors bors bot closed this Apr 12, 2021
@hymm hymm deleted the fix_breakerout_fixed_timestep branch April 18, 2021 18:16
jihiggins pushed a commit to jihiggins/bevy that referenced this pull request Apr 18, 2021
This PR fixes bevyengine#1240, where the ball is escaping the playing field at low framerates.  I did this by moving the movement and physics system into a Fixed Timestep system set and changing the movement steps to a constant.  So we lose the example use of delta_time for changing position, but gain a use of FixedTimestep.

If this is accepted bevyengine/bevy-website#102 will need to be updated to match.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
This PR fixes bevyengine#1240, where the ball is escaping the playing field at low framerates.  I did this by moving the movement and physics system into a Fixed Timestep system set and changing the movement steps to a constant.  So we lose the example use of delta_time for changing position, but gain a use of FixedTimestep.

If this is accepted bevyengine/bevy-website#102 will need to be updated to match.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples 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
Development

Successfully merging this pull request may close these issues.

ball escapes arena in breakout example
4 participants