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

Bump CurrentProtocol #567

Closed
wants to merge 2 commits into from
Closed

Bump CurrentProtocol #567

wants to merge 2 commits into from

Conversation

JustArchi
Copy link
Contributor

@JustArchi JustArchi commented Jun 14, 2018

I updated those by hand if it matters, latest values from beta SteamClient.

Let me know if there is anything else I should commit together with this PR, this worked for me. It doesn't look like a prerequisite for anything right now (managed to get new callbacks I wanted also on old protocol), but always best to track latest one.

@codecov-io
Copy link

codecov-io commented Jun 14, 2018

Codecov Report

Merging #567 into steamclient-beta will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@                Coverage Diff                @@
##           steamclient-beta     #567   +/-   ##
=================================================
  Coverage             22.12%   22.12%           
=================================================
  Files                    86       86           
  Lines                  8666     8666           
  Branches                714      714           
=================================================
  Hits                   1917     1917           
  Misses                 6624     6624           
  Partials                125      125
Impacted Files Coverage Δ
...t2/SteamKit2/Steam/Handlers/SteamUser/SteamUser.cs 25.94% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61a93cf...75b11fb. Read the comment docs.

@yaakov-h
Copy link
Member

Do we know what the side-effects are?

This usually indicates some kind of breaking change.

@JustArchi
Copy link
Contributor Author

I didn't find out any broken functionality yet, but then again I tested only logging in part for now. Would be cool if we had some unit tests for that.

Do you have any idea how we could find out breaking changes? I mean, apart from me putting this into my toys and finding out what broke the hard way 😅. I'll do that the moment I'm done with basic group chat implementation.

@yaakov-h
Copy link
Member

We can only really rely on it being an observable change.

For example, AFAIK we’ve still never seen a CMsgProtobufWrapped.

@JustArchi
Copy link
Contributor Author

JustArchi commented Jun 17, 2018

I have a theory that the protocol change might be related to new chat implementation, if the bump happened in the last beta client. I didn't check what value current stable uses.

I base my theory on the fact that when you set current UI mode to chat of 2 (to enable beta chat), a lot of previously-working callbacks no longer arrive, while you're receiving new beta ones. I'm talking mainly about all callbacks related to friend messages. It also looks like Valve will at best deprecate, at worst entirely remove those by making unusable all of old functions such as sending messages, invite to chat and so on, since right now it's running in compat mode, but I don't see how they'd want to keep that obsolete code working when nobody will practically have access to those functions the moment beta update goes stable. It's definitely not going to be compatible, as new chats use a combination of chat_group_id and chat_id, while old functions base on single SteamID that is Chat account.

I mean, this is the only thing that I noticed myself. It's entirely possible that the change has nothing to do with it, but just saying what I happened to find. I tested current UI mode with both protocol versions and everything seems to be the same (you can enable beta chat on old protcol too). It doesn't look like any existing functionality changed based on that, but we won't know for sure without more people testing that - "it works for me" is not good enough. I think it'd be a good idea to put it into alpha before beta chat makes it into stable, so any potential problems could be fixed in time, with eventual revert if needed. It's better to find out about it sooner than later, especially now when we can still test things and not when beta goes stable, potentially forcing us to newer one (Valve probably keeps very low amount of backwards compatibility, if any, considering that all Steam clients have forced auto-updates).

@yaakov-h
Copy link
Member

I think the bump happened just prior to the new beta. @xPaw do you recall?

FWIW, Valve keep a fairly long trail of backwards compatibility since IIRC game servers don’t auto-update the Steam components, or at least didn’t used to.

@xPaw
Copy link
Member

xPaw commented Jun 17, 2018

@yaakov-h No idea when it happened, I only saw it in the friends ui.

@yaakov-h
Copy link
Member

Just checked the binaries, and current stable uses the bumped version too.

It seems that maybe we already do already know what's in the new protocol: https://github.com/JustArchi/SteamKit/blob/75b11fbc6acbaa1b4dc033c01971979a2a17b02d/Resources/SteamLanguage/steammsg.steamd#L73-L74

😕

If you can re-target this PR against master, and everything works, I'll merge it.

@psychonic do you know what ClientInstanceIDs refers to?

@psychonic
Copy link
Member

I don't know the significance of this, but I assume it's related to the client_instance_id field in the logon request/response messages.

@voided
Copy link
Member

voided commented Jun 17, 2018 via email

@JustArchi JustArchi mentioned this pull request Jun 18, 2018
@JustArchi JustArchi closed this Jun 18, 2018
@JustArchi JustArchi deleted the steamclient-beta branch June 18, 2018 18:31
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.

6 participants