Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

matchmaking servers wrapper #177

Merged
merged 20 commits into from
Jul 30, 2024
Merged

matchmaking servers wrapper #177

merged 20 commits into from
Jul 30, 2024

Conversation

ymo-4
Copy link
Contributor

@ymo-4 ymo-4 commented May 9, 2024

No description provided.

@ymo-4
Copy link
Contributor Author

ymo-4 commented Jul 25, 2024

@Noxime Hi, can you review this when you have time?

Copy link
Owner

@Noxime Noxime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR; This is a lot of work and I'm grateful. I don't have much feedback, just some notes on type convenience. The macro stuff looks quite scary (especially the vtable parts) and I'm not very good at verifying that stuff. I trust you write sensible code 😄

pub max_players: i32,
pub server_version: i32,
pub steamid: u64,
pub last_time_played: std::time::Duration,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

time (in unix time) when this server was last played on (for favorite/history servers)
I think it would be more useful to have this as an Instant instead of a duration since unix epoch. Might be annoying to convert, however 🤷

Copy link
Contributor Author

@ymo-4 ymo-4 Jul 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As i know there's no method to create Instant from milliseconds

Github marked as outdated, just changed std::time::Duration to Duration

pub have_password: bool,
pub secure: bool,
pub bot_players: i32,
pub ping: i32,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either document the unit (milliseconds) or better yet, convert to Duration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, will convert to Duration

src/matchmaking_servers.rs Show resolved Hide resolved
@ymo-4 ymo-4 requested a review from Noxime July 27, 2024 17:39
@ymo-4
Copy link
Contributor Author

ymo-4 commented Jul 27, 2024

Taking all this into account I'm not sure if it's worth at all to return Request from function itself. It will make request non-reusable (since you won't be able to refresh query from outside), etc. but on other hand we'll get rid of that ugly ServerListRequest's inside Mutex'es and as far as i think make possible releasing request automatically or while query is refreshing if user wants this. Do you have any thoughts about that?

Just done rewriting it to see if it will work. You can check it in the history-rewite branch. Looks cleaner and simpler, isn't it? However "autoreleasing" looks a bit weird and hacky (you can see it in add autorelease commit). (upd: rewrote with accessor struct to release method). Also tested it a little bit and it seems that request.refresh_server() sets is_refreshing state as true and set it back to false after a few RunCallbacks calls but do not call any callbacks, which prevents automatically releasing, especially when calling inside a refresh_complete. Since RunCallbacks waiting for our callback function to be done while we are waiting for refresh_server() results we're getting endless waiting. So we need to either remove refresh_server() method or make our own custom callback if it's possible.

What do you think of this rewritten version?

@ymo-4
Copy link
Contributor Author

ymo-4 commented Jul 28, 2024

To summarize what I have for now. There are two slightly different wrappers for the history requests. Both works well and are ready to merge. Each one has it's pros and cons:

mms (this)

Pros:

  • User has (almost) all features and possibilities of the request which steamworks gives
  • User can keep the request alive as long as they want and use it from any threads and places they want

Cons:

  • User must release request by himself
  • Has that ugly Arc<Mutex<ServerListRequest>> everywhere
  • User can't release request while query is refreshing

history-rewrite

Pros:

  • User does not have to worry about releasing the request. It will be released automatically after the refresh_complete callback is called and the query is not refreshing anymore
  • User can release the request by himself even while query is refreshing
  • Code looks cleaner and more "rusty"

Cons:

  • Request is non-reusable in some sense
  • More limitations, less features
  • No ServerListRequest.refresh_server method

Feel free to review both of them, decide which is better and if all is well - merge it! Or maybe both are good and we can merge them like crate features or something, who knows.

Copy link
Owner

@Noxime Noxime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this part of Steamworks 😅 overall though, I think I prefer this approach. Covering more of the features is important, and the "risk" of not releasing a request is still safe, just wasteful. It's a lesser evil; The cons of this approach are less severe than in history-rewrite. We can always revisit this API and rework, if it causes a lot of issues.

Thank you for you work, you have put a lot of effort in!

@Noxime Noxime merged commit d33cf78 into Noxime:master Jul 30, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants