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

Add ValueChange and AxisPairChange variants to ActionDiff #422

Merged

Conversation

WesleyClements
Copy link
Contributor

@WesleyClements WesleyClements commented Dec 6, 2023

In this PR I added an ActionDiff::AxisChanged variant. This is meant to allow for transmitting changes to axis input types across the network. I've updated the relevant systems to handle creating events for this variant on the client and consuming them on the server. I've been using this solution in my own projects for a little bit and it seems to be working but I'm new to rust in addition to the bevy ecosystem so would really love some feedback as to if this seems like a reasonable solution. There are three potentially breaking changes. First I had to remove Eq and Hash from ActionDiff as f32 doesn't implement them. Second I had to add a local resource to the generate_action_diffs system to keep track of if the axis actually changed. Lastly I had to add a requirement that the ID type implement the Hash trait as it is being used a key for a hashmap.

Fixes #342

@WesleyClements
Copy link
Contributor Author

WesleyClements commented Dec 7, 2023

I know this will add an extra minimum necessary bit to distinguish between the variants which can potentially add up especially if sending in bulk/with a lot of players. I've been using bincode for serialization which (for the way I have it configured at least 🤷) seems to use at least a byte for the variant signifier so this change wouldn't increase the overhead. Regardless, this seems to be the most ergonomic approach as it makes it simpler for the user to only have to handle passing/serializing/deserializing one event type between client and server. That being said, an alternative to this might be to create a separate struct for the AxisDiff and send a different event for those changes. That would force the user to handle both events and figure out how to differentiate between them across the network. I'm using bevy_renet for networking and from what I've seen it wouldn't be too difficult to create a second channel to send the AxisDiffs on. That being said, I'm not sure I would actually reduce the data being sent over the network. I could imagine that the client is typically only changing 8-ish inputs each update which would mean that the extra variant would tack on an extra byte per request. I'm making another assumption that the overhead from making a request on two channels is comparable to that extra byte per request. I could be very off here and definitely would like feedback and thoughts but those are my follow up thoughts after sleeping on this PR and panicking that I hadn't thought through it thoroughly enough 😁.

@WesleyClements
Copy link
Contributor Author

WesleyClements commented Dec 7, 2023

I didn't think it through enough. I realize this doesn't handle single axis input types. I will think of a solution to this problem and implement it real quick.

@WesleyClements
Copy link
Contributor Author

WesleyClements commented Dec 7, 2023

I added support for syncing ActionData.value by including a fourth variant. I also had to refactor the generate_action_diffs. It would have been easier if I knew how to know the input type of an action but I just assumed that if the value of an action is 1.0 that it is a binary input. As part of this change I removed the behavior of sending two events when an axis input is pressed. It will no longer send a Pressed event in addition to a AxisPairChanged event. I'm not set on any names for variants. It does seem odd to me that ValueChanged is being sent as the "pressed" event but I guess you don't really press an axis any how.

@WesleyClements WesleyClements changed the title Add AxisChange variant to ActionDiff Add ValueChange and AxisPairChange variants to ActionDiff Dec 7, 2023
@alice-i-cecile alice-i-cecile added the enhancement New feature or request label Dec 7, 2023
@alice-i-cecile
Copy link
Contributor

Good, this seems like a sensible approach! Could you add some more tests to help us spot regressions?

@WesleyClements
Copy link
Contributor Author

Most definitely!

src/systems.rs Show resolved Hide resolved
examples/send_actions_over_network.rs Show resolved Hide resolved
RELEASES.md Outdated Show resolved Hide resolved
@WesleyClements WesleyClements marked this pull request as ready for review December 7, 2023 19:50
@alice-i-cecile alice-i-cecile enabled auto-merge (squash) December 9, 2023 14:32
@WesleyClements
Copy link
Contributor Author

This is now dependent on #424

auto-merge was automatically disabled December 13, 2023 02:30

Head branch was pushed to by a user without write access

@alice-i-cecile alice-i-cecile merged commit 7a92892 into Leafwing-Studios:main Dec 13, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Axis values support for network diffs
2 participants