RFC: feat: Input type must be Default+Serde but not POD #82
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Change Description
(from commit msg)
Remove the bytemuck bounds on Config::Input, and replace them with Default and Serialize + DeserializeOwned.
Both changes are backwards incompatible, though they should be quite easy to resolve for existing users of GGRS (unless they have a philosophical objection to using Serde directly, I suppose).
However, number 2 does have some significant tradeoffs associated with it; in order of least to most bad:
Discussion
Firstly, I am aware that you can already do "custom serialization of an arbitrary input type" (hereafter referred to as
W
) by declaringConfig::Input
as[u8; N]
, with some carefully-chosen-to-be-big-enoughN
; the XOR + RLE that's done on input bytes should make the cost of any extra padding bytes (over and above what's needed to store the arbitrary input type) close to zero, as any long run of trailing zeros would be compressed to just a couple of bytes.Now having said that, requiring various Bytemuck bounds on Input is quite a strong stipulation, and it sure would be nice if you could e.g. use enums with fields ( #40 ), or even just plain old
bool
s in your input type.So that's what this PR gets you: any type you can serialize, you can stick into your
Config::Input
struct - which is nice and convenient.But: I am not exactly sure this is a wise PR to merge, mainly because of tradeoff
C
listed above (B seems manageable, A seems negligible). In fact, I don't think it would make sense to merge this if we don't come up with a good way to mitigate the impact ofC
, because clearly "crash when enough people press enough buttons" is pretty terrible behavior.One approach (let's call it
X
) for mitigatingC
would be to treat the current assert as an error and propagate it all the way up to user code for handling. But I suspect that would be quite messy to do, and what do you do if you get that error at that point? I think there isn't really much in the way of meaningful error handling.Another approach I can think of (
Y
) is that when you set a local player's input, GGRS encodes it together with other inputs this frame so far to check the total size, and if "size of these extra bytes plus the size of other local inputs for this frame so far" exceeds the maximum packet size, then return an error. The user could then choose to drop the input on the floor (or defer it to a later frame) by replacing it with a default input, which would (hopefully) fit.. but there's still a possibility that it wouldn't fit, and then the only sensible thing to do is to crash anyway.(And besides, the "shape" of solution
Y
looks very similar to what's possible today viaW
, but at least in that approach the user is explicitly forced to think about the size of their byte array, rather than attempting to deal with their data not fitting later.Y
does open up the possibility of larger input structs since the check could be done after the XOR + RLE, but that would also make it less predictable when errors occur.)So.. that's where I'm at. I started down this rabbit hole thinking this approach probably wouldn't work, then it turns out it works quite nicely, but it potentially encourages running into a currently-quite-niche edge case - and I haven't got a nice design solution for that.
Still, I figured I'd write up and raise the PR to give others a chance to weigh in (and there's always the chance I've misunderstood something; I'm still new to this crate).