Fix PR Bevy 0.9 Update #38

Vrixyz · 2022-12-02T22:40:06Z

My attempt to help with #36.

What I added with commit 63dc9af:

cargo test passing with bevy 0.9.1

renamed and split register_rollback_type in register_rollback_component and register_rollback_resource
all examples now compile
up dependency to bevy 0.9.1 to use resources.iter()
remove refect_resource mod

What I tested

cargo test.
run the examples (2 players + 1 spectator) on mac.

- renamed and split `register_rollback_type` in `register_rollback_component` and `register_rollback_resource` - cleaned up examples - split register_component and register_resource - up dependency to bevy 0.9.1 to use `resources.iter()`

gschup · 2022-12-03T11:49:41Z

thank you for working on this! The two-player example seems to work, but the spectator example crashes immediately.

cargo run --example box_game_spectator -- --local-port 7002 --num-players 2 --host 127.0.0.1:7000

leads to:

thread 'main' panicked at 'This examples focus on p2p.', examples/box_game/box_game_spectator.rs:90:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', C:\Users\Georg Schuppe\.cargo\registry\src\github.com-1ecc6299db9ec823\bevy_tasks-0.9.1\src\task_pool.rs:273:45

johanhelsing

Got to admit I don't understand all of the reflect stuff. But most of this PR looks sensible to me (though I wrote parts of it myself :P)

Added mostly nitpick comments.

As I understand it what were previously part of the GGRS reflect_resource module is now implemented within Bevy itself. I guess that means we should also delete the reflect_resource module?

You mentioned you didn't do much checking... One thing you could do, is check if the frame count resource in the p2p example works as expected?

EDIT: Also, thanks for picking this up! :)

examples/box_game/box_game_p2p.rs

examples/box_game/box_game_spectator.rs

src/lib.rs

johanhelsing · 2022-12-03T11:48:24Z

src/lib.rs

-        SessionType::SyncTestSession
-    }
-}
+// TODO: more specific name to avoid conflicts?


I think we should probably think about what to name this... It's public API that will be used, and Inputs quite easily conflicts with something else... and is very similar to Bevy's Input.

I think I agree, but at t he same time it's namespaced so it's not a very big issue. What do you think of SessionInputs OR SessionFrameInput ? Also this could use some type documentation.

yeah, it's easy enough to fix... More thinking about it being easy to read.

We have prefixed GGRSError, GGRSConfig. And some have P2P as prefix (P2PSession).

GGRSInputs

P2PInputs

PlayerInputs (matches PlayerHandle which is used to index this resource)

SessionInputs

Just spitballing. Don't really have a strong opinion. I'm also ok with Inputs. What do you think @gschup ?

definitely agree it could use some documentation!

properly naming the Inputs is tough, but I think we should definitely not name it Inputs. I like PlayerInputs quite a bit. I would shy away from naming it GGRSInputs as this struct does not come from the GGRS crate itself.

johanhelsing · 2022-12-03T11:50:20Z

src/lib.rs

@@ -55,7 +54,7 @@ impl Rollback {

 /// Provides unique ids for your Rollback components.
 /// When you add the GGRS Plugin, this should be available as a resource.
-#[derive(Default)]
+#[derive(Resource, Default)]


Suggested change

#[derive(Resource, Default)]

#[derive(Resource, Default)]

#[reflect(Resource)]

What does this accomplish ? wouldn't Resource already have some reflect capabilities ? or is it to enable bevy_egui reflection ?

Hmm.. maybe it's not needed? I think I maybe just made a brainfart and thought you added derive(Reflect).

I guess the real answer is to just check what works :P

Yep, sorry about the noise. We could perhaps add reflect in another PR. It might be useful for debugging.

johanhelsing · 2022-12-03T11:50:54Z

src/lib.rs

-    }
-}
+// TODO: more specific name to avoid conflicts?
+#[derive(Resource, Deref, DerefMut)]


Suggested change

#[derive(Resource, Deref, DerefMut)]

#[derive(Resource, Deref, DerefMut)]

#[reflect(Resource)]

src/world_snapshot.rs

Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>

Vrixyz · 2022-12-07T08:45:13Z

examples/box_game/box_game.rs

@@ -32,7 +32,7 @@ impl Config for GGRSConfig {
 }

 #[repr(C)]
-#[derive(Copy, Clone, PartialEq, Pod, Zeroable)]
+#[derive(Copy, Clone, PartialEq, Eq, Pod, Zeroable)]


Eq was not really necessary but it pleases clippy :)

Vrixyz · 2022-12-07T08:46:14Z

examples/box_game/box_game.rs

-// in order to allow a GGRS `SyncTestSession` to construct a checksum for a world snapshot
-#[derive(Default, Reflect, Hash, Component)]
+// You can also register resources.
+#[derive(Resource, Default, Reflect, Hash)]


I'm not sure if reflect/hash is still needed, I didn't check

My understanding of this is still kind of shallow, but register_rollback_resource seems to need Reflect because it's needed for TypeRegistration::insert...

Or more specifically TypeRegistration::insert needs the TypeData trait to be implemented, which is automatically implemented by deriving Reflect.

I think #[derive(Hash)] and #[reflect(Hash) is not needed. At least the examples, including synctest, run fine without them. Perhaps @gschup would like to confirm?

Deriving and reflecting hash is useful here. It is definitely optional, but encouraged to give SyncTest an actual meaning beyond "checking if this still runs".

I guess we could be more specific about it in the comment... Something like "By deriving Hash and adding reflect(Hash) the resource can automatically be checked for desyncs during a SyncTestSession"?

There is definitely a lot to be improved regarding documentation (see e.g. #35). It might probably even be better to that outside of this PR, in a big documentation commit.

johanhelsing

I tested resource rollback by adding some logging to the synctest session example's FrameCount. Everything works as expected :D

Also ported my tutorial to this branch, and it works fine (though that doesn't really need resource rollback): https://github.com/johanhelsing/extreme_bevy/tree/bevy-0.9-wip

Feels like this is really close now, just some minor nits and some docs for the new resources would be nice. Great work @Vrixyz !

johanhelsing · 2022-12-09T11:18:49Z

examples/box_game/box_game.rs

-// in order to allow a GGRS `SyncTestSession` to construct a checksum for a world snapshot
-#[derive(Default, Reflect, Hash, Component)]
+// You can also register resources.
+#[derive(Resource, Default, Reflect, Hash)]


My understanding of this is still kind of shallow, but register_rollback_resource seems to need Reflect because it's needed for TypeRegistration::insert...

Or more specifically TypeRegistration::insert needs the TypeData trait to be implemented, which is automatically implemented by deriving Reflect.

I think #[derive(Hash)] and #[reflect(Hash) is not needed. At least the examples, including synctest, run fine without them. Perhaps @gschup would like to confirm?

johanhelsing · 2022-12-09T11:26:57Z

src/lib.rs

@@ -55,7 +54,7 @@ impl Rollback {

 /// Provides unique ids for your Rollback components.
 /// When you add the GGRS Plugin, this should be available as a resource.
-#[derive(Default)]
+#[derive(Resource, Default)]


Yep, sorry about the noise. We could perhaps add reflect in another PR. It might be useful for debugging.

johanhelsing · 2022-12-09T11:35:22Z

src/lib.rs

-        SessionType::SyncTestSession
-    }
-}
+// TODO: more specific name to avoid conflicts?


yeah, it's easy enough to fix... More thinking about it being easy to read.

We have prefixed GGRSError, GGRSConfig. And some have P2P as prefix (P2PSession).

GGRSInputs

P2PInputs

PlayerInputs (matches PlayerHandle which is used to index this resource)

SessionInputs

Just spitballing. Don't really have a strong opinion. I'm also ok with Inputs. What do you think @gschup ?

definitely agree it could use some documentation!

examples/box_game/box_game_spectator.rs

examples/box_game/box_game_p2p.rs

Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>

gschup · 2022-12-09T13:21:37Z

I don't think we are missing much here. I think it would be nice to rename Inputs to PlayerInputs and then, we can merge :)

johanhelsing and others added 3 commits November 17, 2022 11:05

Upgrade to Bevy 0.9 and disable resource rollback

0a8e73b

Remove unnecessary Session::None variant

4a9e639

cargo test passing with bevy 0.9.1

63dc9af

- renamed and split `register_rollback_type` in `register_rollback_component` and `register_rollback_resource` - cleaned up examples - split register_component and register_resource - up dependency to bevy 0.9.1 to use `resources.iter()`

Vrixyz changed the title ~~Pr/johanhelsing/36~~ Fix PR #32 Dec 2, 2022

Vrixyz changed the title ~~Fix PR #32~~ Fix PR #36 Dec 2, 2022

Vrixyz changed the title ~~Fix PR #36~~ Fix PR Bevy 0.9 Update Dec 2, 2022

johanhelsing requested changes Dec 3, 2022

View reviewed changes

PR reviews + fix spectator example

d9800aa

Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>

Vrixyz commented Dec 7, 2022

View reviewed changes

johanhelsing reviewed Dec 9, 2022

View reviewed changes

Vrixyz and others added 3 commits December 9, 2022 13:08

Update examples/box_game/box_game_p2p.rs

7849158

Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>

Update examples/box_game/box_game_spectator.rs

d2e3f3b

Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>

Update examples/box_game/box_game_spectator.rs

1cc28cc

Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>

gschup merged commit b730a0b into gschup:main Dec 9, 2022

rename Inputs into PlayerInputs

d2cb4c7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix PR Bevy 0.9 Update #38

Fix PR Bevy 0.9 Update #38

Vrixyz commented Dec 2, 2022 •

edited

Loading

gschup commented Dec 3, 2022

johanhelsing left a comment •

edited

Loading

johanhelsing Dec 3, 2022

Vrixyz Dec 4, 2022

johanhelsing Dec 9, 2022

gschup Dec 9, 2022

johanhelsing Dec 3, 2022

Vrixyz Dec 4, 2022

johanhelsing Dec 4, 2022

johanhelsing Dec 9, 2022

johanhelsing Dec 3, 2022

Vrixyz Dec 7, 2022

Vrixyz Dec 7, 2022

johanhelsing Dec 9, 2022

gschup Dec 9, 2022 •

edited

Loading

johanhelsing Dec 9, 2022

gschup Dec 9, 2022

johanhelsing left a comment

johanhelsing Dec 9, 2022

johanhelsing Dec 9, 2022

johanhelsing Dec 9, 2022

gschup commented Dec 9, 2022

	#[derive(Resource, Default)]
	#[derive(Resource, Default)]
	#[reflect(Resource)]

	#[derive(Resource, Deref, DerefMut)]
	#[derive(Resource, Deref, DerefMut)]
	#[reflect(Resource)]

Fix PR Bevy 0.9 Update #38

Fix PR Bevy 0.9 Update #38

Conversation

Vrixyz commented Dec 2, 2022 • edited Loading

What I tested

gschup commented Dec 3, 2022

johanhelsing left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschup Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanhelsing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschup commented Dec 9, 2022

Vrixyz commented Dec 2, 2022 •

edited

Loading

johanhelsing left a comment •

edited

Loading

gschup Dec 9, 2022 •

edited

Loading