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

Attempt to update for Bevy 0.2 #10

Merged
merged 4 commits into from
Sep 21, 2020
Merged

Conversation

CleanCut
Copy link
Contributor

@CleanCut CleanCut commented Sep 20, 2020

This is my attempt at updating bevy_rapier to work with Bevy 0.2.x

  • On macOS with this PR, the examples all run with a blank white screen and debug output of many lines of queue 21 -- that's not...ideal.
  • On macOS without this PR (using Bevy 0.1.x), the examples were already crashing with this error:
thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', /Users/nathan/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/bevy_text-0.1.3/src/font_atlas_set.rs:53:42

Everything builds, but I don't know if it is functioning correctly. I am able to get a personal project to build with this PR on my mac, but it doesn't seem to do anything. It might be user error in my personal project, or perhaps this upgrade attempt is faulty. I've never used bevy_rapier (or rapier) before today.

Hopefully this proves helpful, as I'd love to add some physics to my project.

@@ -19,7 +19,7 @@ fn main() {
0xF9 as f32 / 255.0,
0xFF as f32 / 255.0,
)))
.add_resource(Msaa { samples: 2 })
Copy link
Contributor Author

@CleanCut CleanCut Sep 20, 2020

Choose a reason for hiding this comment

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

On macOS this crashes with the following error on both Bevy 0.1.x and Bevy 0.2.x:

thread 'main' panicked at 'attachment's sample count 2 is invalid', /Users/nathan/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/wgpu-0.6.0/src/backend/direct.rs:1323:35

..Default::default()
})
.spawn(Camera3dComponents {
transform: Transform::new_sync_disabled(Mat4::face_toward(
Copy link
Contributor Author

@CleanCut CleanCut Sep 20, 2020

Choose a reason for hiding this comment

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

This method was removed in Bevy 0.2.0.

Since the release blog post mentioned disabling sync was an unwanted workaround on the old transform system, I guessed that it was no longer necessary and switched to ::new():

  • Setting a Transform to a specific matrix value (ex: Mat4::look_at()) was extremely cumbersome, and the value would be immediately overwritten unless the user explicitly disabled component syncing.

*translation.0.x_mut() = pos.translation.vector.x * scale.0;
*translation.0.y_mut() = pos.translation.vector.y * scale.0;
rotation.0 = Quat::from_xyzw(rot.i, rot.j, rot.k, rot.w);
transform.set_translation(Vec3::new(pos.translation.vector.x * scale.0, pos.translation.vector.y * scale.0, 0.0));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guessed that the z value for 2d should be 0.0 here.

Copy link
Contributor

Choose a reason for hiding this comment

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

z value is used in bevy for sprite layering, so I think rapier should not touch the value set originally by the user.

@@ -97,9 +98,6 @@ pub fn create_collider_renders_system(
ground_pbr.draw,
ground_pbr.render_pipelines,
ground_pbr.transform,
ground_pbr.translation,
ground_pbr.rotation,
NonUniformScale(scale),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not certain if there's some additional component that's supposed to be added to indicate that there's a non-uniform scale that should be looked at.

@memoryruins
Copy link

memoryruins commented Sep 20, 2020

Thanks for looking into this!

debug output of many lines of queue 21

This is due to a debug print accidentally included in the release bevyengine/bevy#521

the examples all run with a blank white screen

So far that has been my experience, but I haven't had a chance to look into it more yet since last night / this morning.

Although the following "fix" for some previous breakage shouldn't be needed anymore, I tried to apply this diff mentioned in discord. It made no change / continued to be white.

cc @cart (since I think you would be interested in reviewing this anyway).

On macOS without this PR (using Bevy 1.x), the examples were already crashing with this error:

thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', /Users/nathan/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/bevy_text-0.1.3/src/font_atlas_set.rs:53:42

Are you running the examples from the workspace root?
If I run it from bevy_rapier/, the examples run correctly for me.
If I run them from bevy_rapier/bevy_rapier2d/ for example, I can reproduce the panic.
Presumably because it's looking for the resources relative to where its running and the font asset is in bevy_rapier/assets/.


On another note, I noticed that bevy_rapier2d and bevy_rapier3d have a former (before this PR) dev-dependency on bevy_fly_camera that still pulls in 0.1.3, but it doesn't look like it's used anywhere. Perhaps its safe to remove?
I'll note anyway that bevy_fly_camera is being updated to 0.2 in mcpar-land/bevy_fly_camera#2

@CleanCut CleanCut mentioned this pull request Sep 20, 2020
@CleanCut
Copy link
Contributor Author

CleanCut commented Sep 20, 2020

Are you running the examples from the workspace root?

My apologies! I was not running the examples from master in workspace root when I had initially tested. I thought I had tested in the root as well, but either I really didn't or when I did I hit the Msaa error and mistook it for the unwrap() error. The examples do work for me on master on macOS once I fix the Msaa samples error -- it's nice to finally see something working, even if it's not my PR! 😄 🎉

Since I had to fix them for myself to see them work, I went ahead and put up a PR for just that fix in #11

On another note, I noticed that bevy_rapier2d and bevy_rapier3d have a former (before this PR) dev-dependency on bevy_fly_camera that still pulls in 0.1.3, but it doesn't look like it's used anywhere. Perhaps its safe to remove?

Yep, looks like bevy_fly_camera is completely unused, so I pushed up a new commit to remove it. Nothing changed in the behavior of the examples (still just a white screen 😢).

Note: Bevy 0.2.0 itself is broken at the moment until bevyengine/bevy#526 is merged and 0.2.1 is released. You can workaround the breakage by manually adding async-executor = "=1.2.0" to the dependencies (I didn't feel like it merited inclusion to this patch, since the fix should be out shortly).

@CleanCut
Copy link
Contributor Author

Although the following "fix" for some previous breakage shouldn't be needed anymore, I tried to apply this diff mentioned in discord. It made no change / continued to be white.

For convenience of those reading through this pull request, here is the referenced patch for src/physics/plugins.rs:

+ const RAPIER_POST_UPDATE: &str = "rapier_post_update";
  ...
+ .add_stage_before(stage::POST_UPDATE, RAPIER_POST_UPDATE)
+ .add_system_to_stage(RAPIER_POST_UPDATE, physics::sync_transform_system.system());
- .add_system_to_stage(stage::POST_UPDATE, physics::sync_transform_system.system());

@CleanCut
Copy link
Contributor Author

I read through the pull request updating the Bevy transform system and I am fairly convinced I handled all of the transform changes correctly (at least from the Bevy interface side of things). I'm not sure what to look into next.

@cart
Copy link

cart commented Sep 21, 2020

At first glance this all seems in order.

@cart
Copy link

cart commented Sep 21, 2020

I'm playing around with this branch now.

@BorisBoutillier
Copy link
Contributor

Two issues for the blank examples:

  1. The src/render/systems.rs is creating a PbrComponents and then insert each of its component, but missed the new global_transform.
    I suggest to directly add the whole bundle:
--- a/src/render/systems.rs
+++ b/src/render/systems.rs
@@ -89,17 +89,7 @@ pub fn create_collider_renders_system(
                     ..Default::default()
                 };
 
-                commands.insert(
-                    entity,
-                    (
-                        ground_pbr.mesh,
-                        ground_pbr.material,
-                        ground_pbr.main_pass,
-                        ground_pbr.draw,
-                        ground_pbr.render_pipelines,
-                        ground_pbr.transform,
-                    ),
-                );
+                commands.insert(entity, ground_pbr);
  1. The DebugUI no more display properly, because, for a reason I don't know, the asset server is now looking in the sub_crate directories, instead of the current directory of the launch commands.
    Adding a symlink from bevy_rapier2d/assets -> assets and from bevy_rapier3d/assets -> assets , fixes the issue, but seems like a WA

@BorisBoutillier
Copy link
Contributor

Regarding 2. feedback on discord is that this is an expected change :

cart: yeah its because we're now using the CARGO_MANIFEST_DIR as the root when you use cargo run
this way it will always use the folder containing Cargo.toml as the root dir

So the symlink could be the proper fix to avoid to duplicate assets in this case.

@sebcrozet
Copy link
Member

sebcrozet commented Sep 21, 2020

Thank you for looking into this 👍
The suggestions from @Bobox214 look like the right way of completing this PR!

On another note, I noticed that bevy_rapier2d and bevy_rapier3d have a former (before this PR) dev-dependency on bevy_fly_camera that still pulls in 0.1.3, but it doesn't look like it's used anywhere. Perhaps its safe to remove?

Yeah, this was there for testing but should be removed now.

@CleanCut
Copy link
Contributor Author

Thanks, everyone! I've implemented all the suggested fixes, and the demos are all working for me now! I think this is it!

🥳 🎉 🎈

@sebcrozet sebcrozet merged commit 449d785 into dimforge:master Sep 21, 2020
@sebcrozet
Copy link
Member

Thanks!

@sebcrozet
Copy link
Member

The bevy_rapier2d and bevy_rapier3d crates v0.2.0 have been published with these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants