API design change proposal

[gustafla]

This week I been papering over the RocketClient with a new simple API as you may have seen. It's starting to work very well for my upcoming Graffathon entry's engine, but the module has gotten rather complicated, partly due to the current client API design.

I started thinking, what if we make both the "low level" RocketClient and the RocketPlayer no longer own the tracks but work on borrowed tracks instead? This would make it easier to keep track state stashed somewhere else but also cleanly represent a "not connected" state by wrapping the client in an Option.

The design change would also make RocketClient::save_tracks redundant and we should remove it. It's already named confusingly because it's not saving anything, just borrowing a reference to internal track state.

The function signatures would have to change to take a &Tracks or &mut Tracks where needed, and some references to self could probably also be removed (I'm thinking get_track mostly).

[gustafla]

One potential issue with this idea is that if the application code decides to mutate the Vec<Track> in between calls, the client/tracker relationship goes out of sync. This could be mitigated by making Tracks a proper newtype without public mutable access instead of the current type alias.

[gustafla]

One more thing I wish to change: let's invert the low level / simple API hierarchy:

• What is currently in simple would no longer be feature-gated, becomes just rust_rocket::Rocket.
• What is currently rust_rocket::RocketClient, and RocketPlayer would become rust_rocket::lowlevel::Client and Player.

To make this possible, I think we need to go with one of these three options for removing the feature gating:

1. Just hard-depend on bincode. I think this is a bit unidiomatic for a library.
2. Complicate the "simple" API and force the user code to pass in closures for track save/load.
3. Implement binary compatibility with the C library's track files and only offer that instead of serde and bincode

I'm personally in favor of option 2. We could also keep the current simple feature flag but just make it enable Rocket constructor functions, which would pass predefined bincode file IO closures to the actual constructor.

[gustafla]

I started prototyping this in api2 branch

[gustafla]

I think I'm finished with this work. Now it's time to take a break from demo engineering and make an actual production, since I'm at Graffathon :)

I'd like to have a bit of review from @Darksecond for the previously mentioned api2 branch, because this is your original design being changed in a major way.

I decided to leave the track save/load code as a user responsibility after all, but document it in the example program, and put links to it in the relevant documentation items. I think the result of all the recent work will be a very noticeable improvement, and brings this crate much closer to 1.0 quality.

[Darksecond]

The current API so far was just get something out that worked; I'll take a closer look this weekend but in general I'd be OK with an API change as long as it's an improvement :)

[Darksecond]

Especially since this crate was/is never intended to use in sizecoding demo's directly, there's no real restrictions on no-std use for example. If you compare it to https://github.com/demoscene-rs/tiny-gl for example that is much more limited in it's API surface and I'm much more picky about that since I do want it to be no_std compatible

[gustafla]

Allright, thank you for the confirmation. Don't feel pressured to do an exhaustive review, I just wanted to make sure that we're on the same page.

Now that I look at the api2 again after sleeping over it, there are a couple of potential issues.

• It's not sufficiently documented that Tracks must be initially empty when used in client mode. The ideal situation would be that we could send the tracker all tracks and keys and it would start displaying them, but my earlier brief experiment with that failed. I might still look at the Qt editor code and check if there is a protocol to do that, maybe even open a PR in both trackers if I feel like it.
• The Event::NotConnected variant feels like it's in the wrong place. I think it should be removed and replaced with just returning None. Instead, the Rocket type should offer an is_connected function for users who care about it.
• Correspondingly to the first point, the code in Rocket::poll_events that handles reconnections is buggy right now. If we cannot get the tracker synced back to our state, we should wipe our state just as the documentation warn the user. Currently it doesn't clear the tracks, oops.

[gustafla]

send the tracker all tracks and keys and it would start displaying them

Thanks to some discussions at Graffathon, I now know why this won't work: the tracker is a server and may serve multiple clients (e.g. stage lights and stage display). Thus no client should modify data that the other clients may want to keep.

[gustafla]

I fixed all the issues in the latest commits, I think. Initial Tracks doesn't have to be empty, but connecting to a tracker must clear the tracks and now that is what happens under all circumstances. Also removed Event::NotConnected in favor of a is_connected method.