Skip to content

Enable servers and clients to declare compatibility with public servers. #830

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.

Sign up for GitHub

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

Draft
rfortier wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

Enable servers and clients to declare compatibility with public servers. #830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion Code/client/Services/Debug/DebugService.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,12 @@
#include <inttypes.h>
extern thread_local bool g_overrideFormId;

constexpr char kBuildTag[] = "Build: " BUILD_COMMIT " " BUILD_BRANCH " EVO\nBuilt: " __TIMESTAMP__;
#define COMPAT_STRING
#ifdef COMPATIBLE_WITH_BUILD_COMMIT
#define COMPAT_STRING "\nCompatible with protocol version " COMPATIBLE_WITH_BUILD_COMMIT
#endif
constexpr char kBuildTag[] = "Build: " BUILD_COMMIT " " BUILD_BRANCH " EVO\nBuilt: " __TIMESTAMP__ COMPAT_STRING;

static void DrawBuildTag()
{
auto* pWindow = BSGraphics::GetMainWindow();
Expand Down
5 changes: 5 additions & 0 deletions Code/client/Services/Generic/InputService.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include <TiltedOnlinePCH.h>
#include <BranchInfo.h>

#include <Services/InputService.h>
#include <Services/OverlayService.h>
Expand Down Expand Up @@ -103,7 +104,11 @@ void SetUIActive(OverlayService& aOverlay, auto apRenderer, bool aActive)
aOverlay.SetActive(aActive);

// Ensures the game is actually loaded, in case the initial event was sent too early
#ifdef COMPATIBLE_WITH_BUILD_COMMIT
aOverlay.SetVersion(COMPATIBLE_WITH_BUILD_COMMIT);
#else
aOverlay.SetVersion(BUILD_COMMIT);
#endif
aOverlay.GetOverlayApp()->ExecuteAsync("enterGame");

apRenderer->SetCursorVisible(aActive);
Expand Down
5 changes: 5 additions & 0 deletions Code/client/Services/Generic/OverlayService.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include <TiltedOnlinePCH.h>
#include <BranchInfo.h>

#include <Services/OverlayService.h>

Expand Down Expand Up @@ -217,7 +218,11 @@ void OverlayService::SetInGame(bool aInGame) noexcept

if (m_inGame)
{
#ifdef COMPATIBLE_WITH_BUILD_COMMIT
SetVersion(COMPATIBLE_WITH_BUILD_COMMIT);
#else
SetVersion(BUILD_COMMIT);
#endif
m_pOverlay->ExecuteAsync("enterGame");
}
else
Expand Down
13 changes: 11 additions & 2 deletions Code/client/Services/Generic/TransportService.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@

#include <BranchInfo.h>
#include <Services/TransportService.h>

#include <Events/ConnectedEvent.h>
Expand Down Expand Up @@ -112,7 +113,11 @@ void TransportService::OnConsume(const void* apData, uint32_t aSize)
void TransportService::OnConnected()
{
AuthenticationRequest request{};
#ifdef COMPATIBLE_WITH_BUILD_COMMIT
request.Version = COMPATIBLE_WITH_BUILD_COMMIT;
#else
request.Version = BUILD_COMMIT;
#endif
request.SKSEActive = IsScriptExtenderLoaded();
request.MO2Active = GetModuleHandleW(kMO2DllName);

Expand Down Expand Up @@ -211,14 +216,18 @@ void TransportService::HandleAuthenticationResponse(const AuthenticationResponse
// error finding

TiltedPhoques::String ErrorInfo;

ErrorInfo = "{";

TiltedPhoques::String protocolVersion{BUILD_COMMIT};
#ifdef COMPATIBLE_WITH_BUILD_COMMIT
protocolVersion = COMPATIBLE_WITH_BUILD_COMMIT;
#endif

switch (acMessage.Type)
{
case AR::kWrongVersion:
ErrorInfo += "\"error\": \"wrong_version\", \"data\": {";
ErrorInfo += fmt::format("\"expectedVersion\": \"{}\", \"version\": \"{}\"", acMessage.Version, BUILD_COMMIT);
ErrorInfo += fmt::format("\"expectedVersion\": \"{}\", \"protocolVersion\": \"{}\", \"version\": \"{}\"", acMessage.Version, protocolVersion, BUILD_COMMIT);
ErrorInfo += "}";
break;
case AR::kModsMismatch:
Expand Down
23 changes: 18 additions & 5 deletions Code/server/GameServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include <Components.h>
#include <BranchInfo.h>
#include <Components.h>
#include <GameServer.h>
#include <Packet.hpp>

Expand Down Expand Up @@ -183,6 +184,9 @@ GameServer::GameServer(Console::ConsoleRegistry& aConsole) noexcept

UpdateInfo();
spdlog::info("Server {} started on port {}", BUILD_COMMIT, GetPort());
#ifdef COMPATIBLE_WITH_BUILD_COMMIT
spdlog::info("Server is compatible with protocol version {}", COMPATIBLE_WITH_BUILD_COMMIT);
#endif
UpdateTitle();

m_pWorld = MakeUnique<World>();
Expand Down Expand Up @@ -825,9 +829,14 @@ void GameServer::HandleAuthenticationRequest(const ConnectionId_t aConnectionId,
};
#if 1
// to make our testing life a bit easier.
if (acRequest->Version != BUILD_COMMIT)
#ifdef COMPATIBLE_WITH_BUILD_COMMIT
const auto effectiveVersion = COMPATIBLE_WITH_BUILD_COMMIT;
#else
const auto effectiveVersion = BUILD_COMMIT;
#endif
if (acRequest->Version != effectiveVersion)
{
spdlog::info("New player {:x} '{}' tried to connect with client {} - Version mismatch", aConnectionId, remoteAddress, acRequest->Version.c_str());
spdlog::info("New player {:x} '{}' tried to connect with client protocol version {} - Version mismatch", aConnectionId, remoteAddress, acRequest->Version.c_str());
sendKick(RT::kWrongVersion);
return;
}
Expand Down Expand Up @@ -1037,8 +1046,12 @@ void GameServer::UpdateTitle() const
{
const auto name = m_info.name.empty() ? "Private server" : m_info.name;
const char* playerText = GetClientCount() <= 1 ? " player" : " players";

const auto title = fmt::format("{} - {} {} - {} Ticks - " BUILD_BRANCH "@" BUILD_COMMIT, name.c_str(), GetClientCount(), playerText, GetTickRate());
#ifdef COMPATIBLE_WITH_BUILD_COMMIT
constexpr auto protocolVersion = ", protocol version " COMPATIBLE_WITH_BUILD_COMMIT;
#else
constexpr auto protocolVersion = "";
#endif
const auto title = fmt::format("{} - {} {} - {} Ticks - " BUILD_BRANCH "@" BUILD_COMMIT "{}", name.c_str(), GetClientCount(), playerText, GetTickRate(), protocolVersion);

#if TP_PLATFORM_WINDOWS
SetConsoleTitleA(title.c_str());
Expand Down
5 changes: 5 additions & 0 deletions Code/server/Services/ServerListService.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include <BranchInfo.h>
#include <Events/PlayerJoinEvent.h>
#include <Events/PlayerLeaveEvent.h>
#include <Events/UpdateEvent.h>
Expand Down Expand Up @@ -75,7 +76,11 @@ void ServerListService::PostAnnouncement(String acName, String acDesc, String ac
uint16_t aPlayerCount, uint16_t aPlayerMaxCount, String acTagList,
bool aPublic, bool aPassword, int32 aFlags) noexcept
{
#ifdef COMPATIBLE_WITH_BUILD_COMMIT
const std::string kVersion{COMPATIBLE_WITH_BUILD_COMMIT};
#else
const std::string kVersion{BUILD_COMMIT};
#endif
const httplib::Params params{
{"name", std::string(acName.c_str(), acName.size())},
{"desc", std::string(acDesc.c_str(), acDesc.size())},
Expand Down
2 changes: 1 addition & 1 deletion Code/skyrim_ui/src/assets/i18n/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@
},
"ERROR": {
"ERRORS": {
"WRONG_VERSION": "This server expects version {{expectedVersion}} but you are on version {{version}}.",
"WRONG_VERSION": "This server requires protocol version {{expectedVersion}}\n but you are on protocol version {{protocolVersion}}, build {{version}}.\nUpdate your version or run the private server that comes with it.",
"MODS_MISMATCH": "This server has ModPolicy enabled.{{mods}}",
"MODS_MISMATCH_REMOVE": "Please remove the following mods to join:\n{{mods}}",
"MODS_MISMATCH_INSTALL": "Please install the following mods to join:\n{{mods}}",
Expand Down
3 changes: 3 additions & 0 deletions xmake.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,9 @@ before_build(function (target)
bool_to_number[branch == "bluedove"],
bool_to_number[branch == "prerel"])

-- Comment out next line if incompatible with current release, or, fix compatibility version.
contents = contents .. "#if !IS_MASTER\n" .. " #define COMPATIBLE_WITH_BUILD_COMMIT \"v1.8.0\"\n" .. " #endif"
Comment on lines +86 to +87
Copy link
Collaborator

@miredirex miredirex Nov 24, 2025

Choose a reason for hiding this comment

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

I'm still not sure about this:

  1. A developer has to be aware of this when releasing a new version of the mod. And has to find and bump the version contained within this string.
  2. "Comment out... if..." I don't like this. Maybe consider integrating this right into the build pipeline. Like via a build flag, or a custom build mode or something

From my experience, things like this are almost always end up abandoned, especially when new maintainers come

Copy link
Contributor Author

@rfortier rfortier Nov 29, 2025

Choose a reason for hiding this comment

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

You have made it plain you don’t like the PR, pointing out potential problems while ignoring the benefits to the community. You have not offered any constructive criticism that is also feasible to implement without magic.

I object to the fears you are voicing, as the disasters you predict require incompetence on the part of the author for the disaster to come about.

Present an improvement that is feasible. Otherwise, I stand by the design and the code.

Copy link
Member

@Force67 Force67 Nov 29, 2025

Choose a reason for hiding this comment

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

Not a big fan, just because the protocol might be compatible between versions does not mean that other behaviour(s) weren't changed between versions.

And since dev releases don't increment the overall version there is no point in providing workarounds 😅

Copy link
Contributor Author

@rfortier rfortier Nov 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Your first point, "does not mean other behaviour(s) weren't changed" is the point I've been trying to make all along too.

The back-and-forth with Miredirex started over this point, that we should be able to compute if the bits in the protocol have changed. Might be technically possible but that is an inadequate solution because it may be that there is no change in the wire protocol at all, but just a semantic / interpretation / behavior change that renders a new version compatible or not. That requires human review to decide.

Even then it can be ambiguous, or at least a decision as to which way to go. The current Vampire Lord animation fix is a case in point that can be used for a discussion example.

At first, M. said it was incompatible because it was a server-side change, implying a wire-protocol change also. But reviewing the final version showed it was only a behavior change with no wire protocol change. If you had the server-side fix, the Vampire Lord transformation fix would be seen by clients, even older clients. If you used even a newer client with the older server, it would work just fine, you just wouldn't have the animation fix.

So, is it compatible with 1.8.0 servers or not?

I'll argue it is compatible, because that is better for the community. People on a fork/branch with the fix can interoperate with 1.8.0 clients and servers with no issues other than a cosmetic one. When a release comes out with the fix, it will still interoperate with the public servers like PlayTogether. Those providers can update at their leisure, or even skip an upgrade cycle if we think another maintenance release will be "soon."

Everyone wins, IMHO.

Now, it's not quite true with the current state of the PR. Taking into account past discussions with the team, there is a clear bias against dealing with version compatibility. So, the current PR ensures that an IS_MASTER build will never make a compatibility declaration.

But, we could choose to change that; the example I just cited above makes a pretty good case for doing so. Plus, the makeup of the team maintaining the mod and approving PRs has changed. The new guard may come to a different conclusion than the old guard; I'm providing that option.

One last point: I have to do this in the fork anyway. It is a better and more supportable solution than having the fork "lie" that it is v1.8.0 just so it will work with the public servers (which is what the fork used to do, and I just think that is dangerous).

This way it can have a distinct version number while still maintaining compatibility, which is far better for hunting down bugs.

If the PR is rejected, the code can just live on in the fork as a "best we can do" solution, as @miredirex suggested. But there are benefits to other branches or forks if it goes in. Like fixing the fact Dev branches can't have distinct version numbers for each new build without breaking compatibility.


-- fix always-compiles problem by updating the file only if content has changed.
local filepath = "build/BranchInfo.h"
local old_content = nil
Expand Down
Loading