Add serialiable and flatbufferable representations of World and Tank #27
Conversation
server/src/serialization.rs
Outdated
| use crate::world::{Tank, World}; | ||
|
|
||
| pub trait Serializable { | ||
| fn serialize(&self, builder: &mut FlatBufferBuilder, config: &Config) -> Vec<u8>; |
There was a problem hiding this comment.
Serializable takes a flatbuffer because we allocating a new FlatbufferBuilder every time we call this function is extremely inefficient.
I know this is not the API we discussed, but I think this is a valid price to pay.
server/src/serialization.rs
Outdated
| use crate::world::{Tank, World}; | ||
|
|
||
| pub trait Serializable { | ||
| fn serialize(&self, builder: &mut FlatBufferBuilder, config: &Config) -> Vec<u8>; |
There was a problem hiding this comment.
This should not take in a builder. You clear the builder anyway, and the caller of the API should not need to construct a builder
There was a problem hiding this comment.
reset() does not clear owned buffer from FlatBufferBuilder. It will reuse this buffer.
server/src/serialization.rs
Outdated
|
|
||
| let recovered_tank = get_root::<world_generated::Tank>(builder.finished_data()); | ||
|
|
||
| assert_eq!(recovered_tank.pos().unwrap().x(), tank.pos_ref().x); |
There was a problem hiding this comment.
Please use results instead of unwrap
There was a problem hiding this comment.
pos() returns an Option, not Result.
At any rate, an unwrap() in a test is not a big deal, crashing will cause an error which is what we want.
| use crate::math::Position; | ||
|
|
||
| pub struct World { | ||
| tanks: Vec<Tank>, |
There was a problem hiding this comment.
Why does this not have dimensions
There was a problem hiding this comment.
Not necessary for v0.
There was a problem hiding this comment.
This is quite necessary @yizzlez
There was a problem hiding this comment.
Why? There are no boundaries restricting movement in v0.
server/src/main.rs
Outdated
| world.add_tank(Tank::new(1, Position { x: 23.0, y: 54.0 })); | ||
| world.add_tank(Tank::new(2, Position { x: 84.0, y: 34.0 })); | ||
|
|
||
| let world = Arc::new(world.serialize(&mut builder, &Config::new(0))); |
There was a problem hiding this comment.
The maintained state should be the rust WOrld struct, not the Buffer itself. We modify the World struct during processing and only serialize into message when we broadcast to clients
There was a problem hiding this comment.
I did this to interoperate into the existing code as easily as possible. We can start using a real World in a future pr.
| } | ||
|
|
||
| trait Flatbufferable<'a> { | ||
| type Buffer; |
| } | ||
| } | ||
|
|
||
| impl<'a> Flatbufferable<'a> for Tank { |
| player.len(), | ||
| config.player_id | ||
| ); | ||
| return Err(anyhow!( |
There was a problem hiding this comment.
Let's add a TODO to use thiserror for error handling since we can treat this serialization module as its own library not application
There was a problem hiding this comment.
We are using anyhow. Using thiserror contradicts this.
https://old.reddit.com/r/rust/comments/dfs1zk/2019_q4_error_patterns_snafu_vs_errderive_anyhow/
There was a problem hiding this comment.
I don't like treating serialization module as own library because it directly interfaces with our game logic.
There was a problem hiding this comment.
Sire you misunderstand. anyhow is the preference for application code, and thiserror is the preference for library code. Here, you have written a nice serialization library.
There was a problem hiding this comment.
We can table this, it's interfering with velocity. But if the server ever needs different handling for each serialization error type, then we will need to define error types.
There was a problem hiding this comment.
See second comment. The serialization module is not much of a library since it will have a lot of game logic (e.g. sorting between teammates and enemies when serializing tank).
There was a problem hiding this comment.
No other application can use our serialization "library" other than ours.
| use crate::math::Position; | ||
|
|
||
| pub struct World { | ||
| tanks: Vec<Tank>, |
There was a problem hiding this comment.
This is quite necessary @yizzlez
This adds native Rust structs for World and Tank, as well as useful Serializable and Flatbufferable traits.
This allows us to easily manipulate structs on the backend while easily being able to wire information the client.
Test Plan:
covered in serialization.rs . cargo test will test serialization and flatbuffering.