WIP: Make ClientId type configurable by smokku · Pull Request #5 · ErnWong/crystalorb

smokku · 2021-06-14T14:27:28Z

This PR makes client_id type configurable (instead of hardcoded usize type).

I am using net::SocketAddr to identify clients: https://github.com/smokku/soldank/blob/981f49b1/server/src/networking.rs#L35
This change allows me to work directly with connections: HashMap<SocketAddr, Connection> without keeping usize <-> SocketAddr mapping.

There is one FIXME: though, for World::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.

ErnWong · 2021-06-15T00:16:39Z

Thanks for the PR!

It receives client_id and I cannot get it to work without tying World type to NetworkResource trait.

That reminds me: In my use case, my World::CommandType would need to identify which player a command is for (e.g. "JUMP for player 123"). I've been using the client_id usize, which meant that my World implementation was already kinda tied to the network implementation details (which, I agree, we should avoid doing).

Perhaps a third option to your two suggestions might be to make the World trait generic over some ClientIdType, or something along those lines? That way, we can write our Worlds that would work with different network implementations. Then, the Client/Server could substitute NetworkResource::ConnectionHandleType into the ClientIdType generic parameter. This option might make the Client/Server generics a little bit ugly though 😅 .

Personally, I don't think command_is_vaild should go inside NetworkResource since the current NetworkResource abstraction feels like it should only contain the "transport layer" interface. Of course, you're more than welcome to come up with a better abstraction / interpretation of what the NetworkResource should be.

smokku · 2021-06-15T00:35:50Z

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?

ErnWong · 2021-06-16T08:38:41Z

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?

smokku · 2021-06-16T09:27:29Z

Something along the lines of this?

Yup! 👍

Then pass Config to generic traits and extract Config::NetworkResourceType, Config::WorldType as needed there.

ErnWong · 2021-06-16T10:50:36Z

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.

smokku · 2021-06-16T11:03:38Z

wilco

smokku · 2021-06-18T12:29:41Z

Updated PR with new Config trait.
It actually simplifies the code.

If that looks good to you, I will update networking crates.

ErnWong

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.

ErnWong · 2021-06-19T00:21:46Z

+    fn command_is_valid<ClientId>(
+        command: &<Self::ConfigType as Config>::CommandType,
+        client_id: ClientId,
+    ) -> bool;


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.

Of course it should use ConnectionHandleType. With all the changes I missed it. ;-)

ErnWong · 2021-06-19T01:29:46Z

-    /// entire [`World`] structure and performing interpolation on all your state variables.
-    type DisplayStateType: DisplayState;
+    /// Configuration parameters that tweak how CrystalOrb works.
+    type ConfigType: Config;


What are you thoughts on

A: Having a Client<World> with World containing the associated type Config as you've done here, compared to

B: Having a Client<Config> with Config containing the associated type World<Config>?

Also, what are your thoughts on

C: Hoisting Command, Snapshot and DisplayState into Config as you've done here, compared to

D: Keeping Command, Snapshot and DisplayState inside World?

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.

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.

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 😅

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?

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. 😸

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.

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)

I haven't committed it, as it was going nowhere 😉

ErnWong · 2021-06-19T03:00:29Z

        seconds_since_startup: f64,
-        net: &mut NetworkResourceType,
+        net: &mut <WorldType::ConfigType as Config>::NetworkResourceType,
+        client_id: <<WorldType::ConfigType as Config>::NetworkResourceType as NetworkResource>::ConnectionHandleType,


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?

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.

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.

smokku · 2021-06-19T09:15:12Z

I love that we no longer need to keep copies of Config structs in memory.

Yup. That was my wow-moment 😄

ValorZard · 2021-06-20T23:10:05Z

Is this ready to be merged?

smokku · 2021-06-21T05:45:16Z

Is this ready to be merged?

Not yet. I am still working on implementing latest changes we discussed.

smokku · 2021-06-21T09:57:47Z

Sorry, I give up.
This is way over my Rust knowledge and time budget.

ErnWong · 2021-06-21T10:00:36Z

Sorry, I give up.
This is way over my Rust knowledge and time budget.

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).

ValorZard · 2021-06-21T10:00:50Z

Huh. What caused you to give up? Also, thanks for trying!

smokku · 2021-06-25T10:48:40Z

I hope you've got what you needed for Soldank (if not, let me know and I can see if I can help).

Yup. Thanks.
I really like the structuring with commands, snapshots and display state - will definitely adopt it.

ErnWong · 2021-07-10T23:37:26Z

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 NetworkResource unaware about the World's details, but issue #7 (and PR #9) made me realise that NetworkResource implementations already kinda need to be aware of the World because those implementations need to set up the appropriate channels for each message type anyway. Trying to keep the NetworkResource trait syntactically decoupled from World would just be hiding their semantic coupling, I think.

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 😄

smokku · 2021-07-19T18:15:48Z

I had a feeling there is a smaller, more focused library hiding inside CrystalOrb, so I started again.
This time from scratch, copying only bits I needed and commenting-out the rest.
https://github.com/smokku/soldank/tree/581b4f446b2cf5264f4c25f4cc2eaa1c0bfc192a/shared/src/orb

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 serde. The library does not care what means the integration layer is using to push messages to/from client.

Bonus point - I do not require nightly compiler anymore.

P.S. This is not a direct port. I am experimenting with removing TimeSync and Stages.

I believe that explicit TimeSync is not needed and may be accomplished as a side effect of exchanging actual useful messages.
I believe staging the simulation is a job for the handshake protocol happening before the actual simulation.

Make ClientId type configurable

52b334e

Create Config trait

8c80833

ErnWong requested changes Jun 19, 2021

View reviewed changes

smokku closed this Jun 21, 2021

smokku deleted the client-id-type branch July 19, 2021 18:11

ErnWong mentioned this pull request Sep 25, 2021

Update readme with examples and related libraries #15

Closed

Conversation

smokku commented Jun 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErnWong commented Jun 15, 2021

Uh oh!

smokku commented Jun 15, 2021

Uh oh!

ErnWong commented Jun 16, 2021

Uh oh!

smokku commented Jun 16, 2021

Uh oh!

ErnWong commented Jun 16, 2021

Uh oh!

smokku commented Jun 16, 2021

Uh oh!

smokku commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErnWong left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

smokku Jun 21, 2021 • 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!

smokku commented Jun 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ValorZard commented Jun 20, 2021

Uh oh!

smokku commented Jun 21, 2021

Uh oh!

smokku commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErnWong commented Jun 21, 2021

Uh oh!

ValorZard commented Jun 21, 2021

Uh oh!

smokku commented Jun 25, 2021

Uh oh!

ErnWong commented Jul 10, 2021

Uh oh!

smokku commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

smokku commented Jun 14, 2021 •

edited

Loading

smokku commented Jun 18, 2021 •

edited

Loading

ErnWong left a comment •

edited

Loading

smokku Jun 21, 2021 •

edited

Loading

smokku commented Jun 19, 2021 •

edited

Loading

smokku commented Jun 21, 2021 •

edited

Loading

smokku commented Jul 19, 2021 •

edited

Loading