gschup / ggrs

GGRS is a reimagination of GGPO, enabling P2P rollback networking in Rust. Rollback to the future!
Other
507 stars 25 forks source link

Derive debug for SessionBuilder #47

Closed johanhelsing closed 1 year ago

johanhelsing commented 1 year ago

When starting a ggrs session, it's often useful to log the settings used, especially on wasm where proper debugging is more difficult.

This enables the following:

        info!("Starting p2p session: {session_builder:#?}");
        let session = session_builder
            .start_p2p_session(socket)
            .expect("failed to start ggrs session");

image

It adds a Debug trait bound on Config::Address and Config::Input... The one on Address is probably not the end of the world, most addresses could probably be logged in some way... Input, however, is maybe a bit less fortunate?

gschup commented 1 year ago

I really dislike the fact that we would force Debug on the Input with this. I feel like we should actually implement debug on the sessionbuilder manually and skip parts that would propagate the debug trait bounds.

Like this: https://doc.rust-lang.org/std/fmt/trait.Debug.html

gschup commented 1 year ago

I am still not a big fan on forcing Debug on the traits we interface with. Now only type Address: Clone + PartialEq + Eq + Hash + Send + Sync + Debug; remains. I am not experienced enough to see if this Debug will ever be a problem to someone.

It does not seem like there is a good solution to provide e.g. conditional debug prints. What do you think?

johanhelsing commented 1 year ago

I cleaned up the PR a little.

Agree if feels kind of wrong, but it also feels wrong to have to maintain it by hand... And seeing the addresses in debug output could really be useful.

One option would be to add the trait bound for Address, but remove it if/when someone complains about it being in the way? Being pre 1.0 and all.

I don't have any strong feelings about this going in, though. Feel free to close it if you'd rather not have it!

gschup commented 1 year ago

I agree. For now, let's put it in and see how long it takes until someone complains :)