Support (fieldless) enums in `Config::Input` #41

Vrixyz · 2022-11-09T17:06:38Z

Improves #40.

in bytemuck, Pod trait was split in multiple traits in release 1.9.0 https://github.com/Lokathor/bytemuck/blob/main/changelog.md#190
NoPadding renamed into NoUninit: Support unions in derive macros Lokathor/bytemuck#94
There is no benchmarks, I think the impact should be very minimal, but I figured I would bring it up.

Vrixyz · 2022-11-09T17:07:56Z

src/frame_info.rs

+    I: Copy
+        + Clone
+        + PartialEq
+        + bytemuck::NoUninit
+        + bytemuck::CheckedBitPattern
+        + bytemuck::Zeroable,


Not sure PlayerInput needs all those type constraints, as tests are passing without. I think it would fail before when setting the Config::Input. That said, it makes sense to have the same constraints so I'm ok with that.

bytemuck::Zeroable is a key constraint used e.g. here.

The documentation on bytemuck::NoUninit recommends implementing bytemuck::CheckedBitPattern (link).

Vrixyz · 2022-11-09T17:08:11Z

src/input_queue.rs

@@ -11,9 +11,9 @@ pub(crate) struct InputQueue<T>
 where
    T: Config,
 {
-    /// The head of the queue. The newest `GameInput` is saved here      
+    /// The head of the queue. The newest `PlayerInput` is saved here      


Added renames from GameInput to PlayerInput (could be slit in another PR..)

Vrixyz · 2022-11-09T17:09:37Z

tests/enum_input.rs

+pub enum EnumInput {
+    Val1,
+    Val2,
+}


For now, it only supports fieldless enums, because NoUninit doesn't support fields, it might be technically possible to add this support upstream in bytemuck ?

I'm wondering if NoUninit is actually needed, and if yes, could MaybeUninit replace it ? 🤔 I'm gonna explore this idea.

Please do! Sounds exciting! :)

I gave it a small attempt, I think it's a good direction, but our problem is bytes_of used here

ggrs/src/network/protocol.rs

Line 54 in 0d70f45

let size = bytemuck::bytes_of(&zeroed.input).len() * num_players;

, which has a type restriction on NoUninit, but it has a lot of opaque safety constraints. I feel like the constraints could be relaxed to support enum with fields, but it might need quite some work from bytemuck, such as Lokathor/bytemuck#84 or Lokathor/bytemuck#55.

I'm gonna start a discussion with bytemuck and see where it goes from there.

Since I was reading thru this PR anyway, just a small note: bytes_of is used elsewhere so this doesn't solve the problem, but instead of creating a zeroed object just to call bytes_of(&zeroed).len() * num_players here, you could forego all of that and simply call core::mem::size_of::<PlayerInput<T::Input>>() * num_players to get the size

thanks for the comment! good catch, that definitely seems like a smarter solution.

tests/enum_input.rs

Vrixyz · 2022-11-09T17:15:25Z

src/network/protocol.rs

@@ -86,7 +86,7 @@ impl InputBytes {
        for p in 0..num_players {
            let start = p * size;
            let end = start + size;
-            let input = *bytemuck::try_from_bytes::<T::Input>(&self.bytes[start..end])
+            let input = *bytemuck::checked::try_from_bytes::<T::Input>(&self.bytes[start..end])


That's an important change.

Can you elaborate what is actually checked here?

Sure, first, we need to understand the behaviour before this change:

With the restriction "AnyBitValid", enums could not work because an enum cannot have "any" bits ; mainly because its type flag, but probably other types are concerned (references, bool...)

Nothing is really checked if the type is "AnyBitPattern" though

the "checked" prefixed leads to:
https://github.com/Lokathor/bytemuck/blob/6b1b7cecd32902b240a8e8593534600a09269d14/src/checked.rs#L209 ; where there is additional checks in case there are not valid bit patterns 😅.

All "AnyBitPatterns" implement "CheckedBitPattern" though, so no additional checks are performed for most types :). (https://github.com/Lokathor/bytemuck/blob/6b1b7cecd32902b240a8e8593534600a09269d14/src/checked.rs#L143)

gschup · 2022-11-13T14:20:25Z

I like this. @Vrixyz what do you think: Is there anything else you want to add/change before we merge this?

Vrixyz · 2022-11-13T14:45:42Z

I'd like to make tests code quality better, I duplicated a lot of code and there may even be dead code.

A first step could be to:

merge this once a minimal cleanup is done
make a follow up issue to make more agressive improvements to the tests (generic tests over "CheckedBitPattern" ?)
make an independent followup issue to attempt support for enums with fields.

remove dead code add enum prefix to 'duplicated' structs separated enum tests from non-enum

Vrixyz · 2022-11-13T20:48:54Z

tests/test_p2p_session.rs

@@ -115,7 +115,7 @@ fn test_advance_frame_p2p_sessions() -> Result<(), GGRSError> {
    assert!(sess1.current_state() == SessionState::Synchronizing);
    assert!(sess2.current_state() == SessionState::Synchronizing);

-    for _ in 0..10 {
+    for _ in 0..50 {


@gschup I would be happy to revert that, but on a 2015 macos, even 0..30 lead to sometimes failures. I figured it's a small price to pay to make the tests a little bit more stable. Eventually we would probably want a timeout, but it's too far from this Pull Request scope.

I find this to be very odd. This never was an issue for me, but network packets are mysterious :D

Vrixyz · 2022-11-13T20:51:59Z

tests/test_synctest_session_enum.rs

+use ggrs::{GGRSError, SessionBuilder};
+
+#[test]
+fn test_enum_advance_frames_with_delayed_input() -> Result<(), GGRSError> {


as said in comment, I would have like to provide more tests, but it's creating a lot of duplicated code, so I figured we could merge minimal tests, and then discuss a better test strategy:

I tried a generic GameStub, but all the data structures are quite tied, and the hardest to make generic is the Config implementation.

macros could be another idea, but they can be controversial.

Vrixyz · 2022-11-13T21:35:38Z

Cargo.toml

@@ -21,7 +21,7 @@ rand = "0.8"
 bitfield-rle = "0.2"
 parking_lot = "0.11"
 instant = "0.1"
-bytemuck = {version = "1.7", features = ["derive"]}
+bytemuck = {version = "1.9", features = ["derive"]}


Pod trait was split in multiple traits in release 1.9.0 https://github.com/Lokathor/bytemuck/blob/main/changelog.md#190

Vrixyz · 2022-11-13T21:59:01Z

I like this. @Vrixyz what do you think: Is there anything else you want to add/change before we merge this?

I'm done with this Pull Request, I'll address feedbacks only now :)

Thanks to fu5ha from gschup#41 (comment)

ease trait bounds on Input to support fieldless enums; gschup#40

08f2c26

Vrixyz commented Nov 9, 2022

View reviewed changes

tests/enum_input.rs Outdated Show resolved Hide resolved

Vrixyz commented Nov 9, 2022

View reviewed changes

gschup linked an issue Nov 10, 2022 that may be closed by this pull request

Support enum in Config::Input #40

Closed

tests: cleanup input enum tests

dbb5803

remove dead code add enum prefix to 'duplicated' structs separated enum tests from non-enum

Vrixyz commented Nov 13, 2022

View reviewed changes

This was referenced Nov 13, 2022

Support enum in Config::Input #40

Closed

Derive support for pod structs with enums in them? Lokathor/bytemuck#84

Open

Vrixyz added a commit to Vrixyz/ggrs that referenced this pull request Nov 14, 2022

fix: removed an allocation in InputBytes::zeroed

de23890

Thanks to fu5ha from gschup#41 (comment)

Vrixyz mentioned this pull request Nov 14, 2022

fix: removed an allocation in InputBytes::zeroed #42

Merged

gschup merged commit 5d824b6 into gschup:main Nov 14, 2022

This was referenced Nov 15, 2022

Support enum with fields in Config::Input #43

Closed

Support enum with fields in Config::Input #44

Closed

Support (fieldless) enums in Config::Input #41

Support (fieldless) enums in Config::Input #41

Uh oh!

Conversation

Vrixyz commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vrixyz Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vrixyz Nov 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fu5ha Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vrixyz Nov 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gschup commented Nov 13, 2022

Uh oh!

Vrixyz commented Nov 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gschup Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vrixyz commented Nov 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Support (fieldless) enums in `Config::Input` #41

Support (fieldless) enums in `Config::Input` #41

Vrixyz commented Nov 9, 2022 •

edited

Loading

Vrixyz Nov 9, 2022 •

edited

Loading

Vrixyz Nov 13, 2022 •

edited

Loading

fu5ha Nov 14, 2022 •

edited

Loading

Vrixyz Nov 13, 2022 •

edited

Loading

gschup Nov 14, 2022 •

edited

Loading

Vrixyz commented Nov 13, 2022 •

edited

Loading