Conversation
Hi, Thank you very much for the PR! 🙏 I'll try to review it this week.
As long as all existing tests pass, we can merge. If it does cause problems, we can address them separately (incl. adding relevant test/benchmark) Of course, I am nevertheless interested to understand the potential problems and what needs to be done to avoid them. But I won't block this PR just because we don't know everything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks, @Canleskis 🙏
🎉 This issue has been resolved in version 4.0.0-alpha.4 🎉 |
🎉 This issue has been resolved in version 4.0.0 🎉 |
@@ -70,6 +71,7 @@ pub struct ColliderHandle(geometry::ColliderHandle); | |||
impl Plugin for RapierPlugin { | |||
fn build(&self, app: &mut App) { | |||
app.add_plugin(heron_core::CorePlugin) | |||
.add_plugin(TimePlugin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If application adds TimePlugin
, this addition breaks time delta. You can reproduce with following application:
use bevy::app::ScheduleRunnerSettings;
use bevy::log::LogPlugin;
use bevy::prelude::*;
use std::time::Duration;
use bevy::time::TimePlugin;
fn main() {
App::new()
.insert_resource(ScheduleRunnerSettings::run_loop(Duration::from_secs(1)))
.add_plugins(MinimalPlugins)
.add_plugin(LogPlugin)
// .add_plugin(TimePlugin) // uncomment and see the difference
.add_system(log_time)
.run();
}
fn log_time(time: Res<Time>) {
info!("time: {:?}", time.delta());
}
Outputs:
Without extra TimePlugin
:
2022-11-10T00:35:32.996089Z INFO bevy_test: time: 0ns
2022-11-10T00:35:34.007733Z INFO bevy_test: time: 1.0119971s
2022-11-10T00:35:35.012710Z INFO bevy_test: time: 1.0049682s
2022-11-10T00:35:36.018760Z INFO bevy_test: time: 1.0060325s
With extra TimePlugin
:
2022-11-10T00:33:18.515528Z INFO bevy_test: time: 1µs
2022-11-10T00:33:19.527569Z INFO bevy_test: time: 1.1µs
2022-11-10T00:33:20.529827Z INFO bevy_test: time: 1.7µs
2022-11-10T00:33:21.540661Z INFO bevy_test: time: 1.1µs
I'd suggest to remove this line. Applications should add all required plugins before adding heron plugins.
Bevy 0.8's GlobalTransform is now backed by an Affine3A, in this PR I just naively convert it back to a transform, rotation and scale when needed using the to_scale_rotation_translation() method. This could cause problems (?), as discussed here: dimforge/bevy_rapier#219
I have not taken the time to work out what problems this could cause, but I assume there needs to be some checks done in some places before assigning to the GlobalTransform.