WIP: Make ClientId type configurable #5
Conversation
|
Thanks for the PR!
That reminds me: In my use case, my Perhaps a third option to your two suggestions might be to make the Personally, I don't think |
|
I really miss OCaml's module-level generics, where you can configure module on your own types. Maybe we could put these types to Config trait and pass it to World, NetworkResourse etc? |
Something along the lines of this? pub trait Config {
type NetworkResourceType: NetworkResource;
type WorldType: World<NetworkResourceType::ConnectionHandleType>;
const OTHER_PARAMETERS_FROM_THE_OLD_CONFIG_STRUCT: f64; // or as functions
// etc.
}
pub struct Client<C: Config> { /* ... */ }
pub struct Server<C: Config> { /* ... */ }Or were you thinking of something different? |
Yup! 👍 Then pass |
|
Cool! Feel free to add it to this PR, or, if that's too much work, let me know and I'd be happy to take over. |
|
wilco |
|
Updated PR with new If that looks good to you, I will update networking crates. |
There was a problem hiding this comment.
Nice. I love that we no longer need to keep copies of Config structs in memory.
With this change, we might be able to remove the use of
#![feature(const_fn_floating_point_arithmetic)]since, if I remember correctly, it was originally used to allow Config::new to be used as a constant function.
I'm not too sure about the way we're structuring the World and Config traits (see other comment), which I would be keen to hear your thoughts on.
| fn command_is_valid<ClientId>( | ||
| command: &<Self::ConfigType as Config>::CommandType, | ||
| client_id: ClientId, | ||
| ) -> bool; |
There was a problem hiding this comment.
Does ClientId need to be generic a generic parameter of this function, or can it use Self::ConfigType::NetworkResourceType::ConnectionHandleType?
I'm thinking that CommandType will be tied to the specific ConnectionHandleType, so both CommandType and client_id need to be the same type in order to check that the command is valid. Making ClientId generic for this command_is_valid function without any additional bounds might make it difficult to implement, as the CommandType's specific choice of client identification could not be compared with all possible ClientId types.
There was a problem hiding this comment.
Of course it should use ConnectionHandleType. With all the changes I missed it. ;-)
| pub trait World: Stepper + Default + Send + Sync + 'static { | ||
| /// The command that can be used by the game and the player to interact with the physics | ||
| /// simulation. Typically, this is an enum of some kind, but it is up to you. | ||
| type CommandType: Command; | ||
|
|
||
| /// The subset of state information about the world that can be used to fully recreate the | ||
| /// world. Needs to be serializable so that it can be sent from the server to the client. There | ||
| /// is nothing stopping you from making the [`SnapshotType`](World::SnapshotType) the same type | ||
| /// as your [`World`] if you are ok with serializing the entire physics engine state. If you | ||
| /// think sending your entire [`World`] is a bit too heavy-weighted, you can hand-craft and | ||
| /// optimise your own `SnapshotType` structure to be as light-weight as possible. | ||
| type SnapshotType: Debug + Clone + Serialize + DeserializeOwned + Send + Sync + 'static; | ||
|
|
||
| /// The subset of state information about the world that is to be displayed/rendered. This is | ||
| /// used by the client to create the perfect blend of state information for the current | ||
| /// rendering frame. You could make this [`DisplayState`](World::DisplayStateType) the same | ||
| /// structure as your [`World`] if you don't mind CrystalOrb making lots of copies of your | ||
| /// entire [`World`] structure and performing interpolation on all your state variables. | ||
| type DisplayStateType: DisplayState; | ||
| /// Configuration parameters that tweak how CrystalOrb works. | ||
| type ConfigType: Config; |
There was a problem hiding this comment.
What are you thoughts on
- A: Having a
Client<World>withWorldcontaining the associated typeConfigas you've done here, compared to - B: Having a
Client<Config>withConfigcontaining the associated typeWorld<Config>?
Also, what are your thoughts on
- C: Hoisting
Command,SnapshotandDisplayStateintoConfigas you've done here, compared to - D: Keeping
Command,SnapshotandDisplayStateinsideWorld?
For A vs B, I'm leaning towards B because I think it better documents the fact that World is decoupled from the choice of NetworkResource (since, as you said, the World should not know about the networking implementation details). I acknowledge that A looks simpler than B in some ways though.
For C vs D, I'm leaning towards D because I think it better documents the coupling relationships between the different types: World is highly coupled with Command, Snapshot and DisplayState, but loosely coupled with NetworkResource. If we chose C for example, then whenever we want to swap out the NetworkResource and reuse the same underlying World struct, we'd probably swap out the Config. However, swapping out the Config would also allow swapping out the Command, which I don't think makes sense since I think the World implementation is inherently tied to the specific Command type.
Sorry, I might be overthinking about it 😅 , especially since I'm relatively new to Rust and might be misunderstanding how generics/traits are supposed to be used. If you think A and C are better choices or they're good enough, I'd be happy to keep things as is. I'd still be keen to hear you thoughts though.
There was a problem hiding this comment.
The sore point here is Command which is coupled both with World (via validate) and NetworkResource (via client_id).
It does not belong wholly to World nor to Network, thus the idea with putting it into separate thing - Config.
I moved all configuration to Config for ease of use. But I do see value in keeping World-only things in World for documentation purpose.
I agree with B as it shows that Client depends on both World and Network (via Config).
I will reshuffle stuff to remove world/network dependencies.
There was a problem hiding this comment.
The sore point here is Command which is coupled both with World (via validate) and NetworkResource (via client_id).
Good point. Do you think the rest of the World should also depend on this client_id the same way as Command, or nah not really? I just remembered that, in another project I've been working on, I've been using client_id to identify which rigid body belongs to which player, for example, which helped me apply the right command to the right rigid body.
I think, if all those World, Command, Snapshot and DisplayState were to depend on client_id, it might be nice to lump them together underneath World. Since they only depend on some way to identify clients/players, and otherwise they don't actually need to know about the NetworkResource, I was thinking that maybe World could could be parameterised by this ClientId, and only make the connection between ClientId and ConnectionHandleType in the Config level. Thoughts on this?
I moved all configuration to Config for ease of use. But I do see value in keeping World-only things in World for documentation purpose.
True, I can see how both ways have their merits 😅
There was a problem hiding this comment.
I think, if all those
World,Command,SnapshotandDisplayStatewere to depend onclient_id, it might be nice to lump them together underneathWorld. Since they only depend on some way to identify clients/players, and otherwise they don't actually need to know about theNetworkResource, I was thinking that maybeWorldcould could be parameterised by thisClientId, and only make the connection betweenClientIdandConnectionHandleTypein theConfiglevel. Thoughts on this?
Nice catch. The point here is that both Network and World has a relation to external entity - Client. Both need a common way of identifying clients, which we should share via Config.
i will try sharing only that and see what happens. 😸
There was a problem hiding this comment.
I will reshuffle stuff to remove world/network dependencies.
Unfortunately after a long struggle I don't see a way of decoupling these.
WorldType::ConfigType::ConnectionHandleType is not the same as NetworkResourceType::ConfigType::ConnectionHandleType and compilation fails.
I don't see a way of making these the same type without putting everything in the same trait.
There was a problem hiding this comment.
Darn, that's a shame, but thank you for taking your time to attempt it anyway.
(If you haven't deleted/undo'ed your refactor attempt but haven't committed yet, I think it'll be awesome to have it committed (even if it'll be reverted in the next commit) to record your efforts for future curious readers)
There was a problem hiding this comment.
I haven't committed it, as it was going nowhere 😉
| &mut self, | ||
| delta_seconds: f64, | ||
| seconds_since_startup: f64, | ||
| net: &mut NetworkResourceType, | ||
| net: &mut <WorldType::ConfigType as Config>::NetworkResourceType, | ||
| client_id: <<WorldType::ConfigType as Config>::NetworkResourceType as NetworkResource>::ConnectionHandleType, |
There was a problem hiding this comment.
Are we intentionally shifting the responsibility to users of this library to figure out what their client_id is?
Previously, the client_id was automatically detected by the ClockSyncer so that the users of this library won't need to. I suppose, if you're using SocketAddr, the clients can figure out their cliend_id themselves, but this might not be the case for other ConnectionHandleTypes. Would it be compatible with your use case if we tried keeping the original auto-detection-of-client-id feature?
There was a problem hiding this comment.
In the first version of PR this was needed as it could not be determined without external information.
I do see the value of passing it explicitly internally, but I agree it could be removed from external API surface.
There was a problem hiding this comment.
Doh.
I tried changing that and I need to add client_id back to ClockSyncMessage - which makes it parametrized type, which ripples through the code even more.
Yup. That was my wow-moment 😄 |
|
Is this ready to be merged? |
Not yet. I am still working on implementing latest changes we discussed. |
|
Sorry, I give up. |
Fair enough. Thanks for your efforts though! I hope you've got what you needed for Soldank (if not, let me know and I can see if I can help). |
|
Huh. What caused you to give up? Also, thanks for trying! |
Yup. Thanks. |
|
In case anyone wants to pick up this PR in the future: I just realised that issue #7 changes some of the design decisions we were making here. In this #5 PR, we were trying to keep Dammit, I was looking forward to trying out some Rust trait magic in my spare time, but it looks like we might not need to anymore 😄 |
|
I had a feeling there is a smaller, more focused library hiding inside CrystalOrb, so I started again. The trick is to ditch the network interfacing completely. Reverse the paradigm and instead of providing abstraction to interface the network, provide a way for integration to pull/push snapshots&commands: https://github.com/smokku/soldank/blob/581b4f446b2cf5264f4c25f4cc2eaa1c0bfc192a/shared/src/orb/server.rs#L32 The added value is dropping the dependency on Bonus point - I do not require P.S. This is not a direct port. I am experimenting with removing TimeSync and Stages.
|
This PR makes
client_idtype configurable (instead of hardcodedusizetype).I am using
net::SocketAddrto identify clients: https://github.com/smokku/soldank/blob/981f49b1/server/src/networking.rs#L35This change allows me to work directly with
connections: HashMap<SocketAddr, Connection>without keepingusize<->SocketAddrmapping.There is one
FIXME:though, forWorld::command_is_valid(). It receives client_id and I cannot get it to work without tying World type to NetworkResource trait. I don't want to do that, as World should not need to know networking implementation details. I'm not really sure where this method belongs. If I put it in NetworkResource, it will need to know World::CommandType type, which is the same problem, just reversed.