Make some client addons optional if server addon not present by BobbyTheCatfish · Pull Request #90 · Extremelyd1/SSMP

BobbyTheCatfish · 2026-08-01T22:59:01Z

When validating addons, addons that can be disabled are disabled instead of preventing connection. Adds a new ClientAddon type called OptionalClientAddon, which is similar to a TogglableClientAddon, with the main difference being that OptionalClientAddon cannot be disabled via commands, unlike TogglableClientAddon.

An example implementation can be found in this git diff

Extremelyd1

Thanks for the PR Bobby! I'm sure this is a welcome change for people that just install any addon they can find and still hope to connect to servers 😛

I've left some comments that question maintainability and whether it can be a bit more simplified.

Extremelyd1 · 2026-08-02T09:34:10Z

+/// <summary>
+/// Abstract class for a client addon that can be disabled by the server. Extends <see cref="ClientAddon"/>.
+/// </summary>
+public abstract class OptionalClientAddon : ClientAddon {


Is it strictly necessary to make this a separate class to TogglableClientAddon? What are the differences between them?

If the server reports to not have an addon that the client does, but that addon is a TogglableClientAddon, can't we disable that client addon, or do we somehow require the OptionalClientAddon for this?

In a similar vein, I'm wondering if the extra fields DisabledByServer and DisabledByClient are necessary. Does the distinction on who disabled it matter in any case?

The togglable addons are turned on and off by the user via a command, and there is a setting that keeps disabled addons turned off until they are manually enabled. If this functionality was meant to be a way to achieve what I'm adding, then that makes sense. Otherwise they'd need to be kept separate since some addons shouldn't be disabled in the middle of a session.

If this functionality was meant to be a way to achieve what I'm adding, then that makes sense
I don't understand what you are saying here.

some addons shouldn't be disabled in the middle of a session
All toggleable addons should be able to be disabled in the menu, but naturally not in-game, because then they are running and if they require networking that would break everything.

So shouldn't it be possible to merge OptionalClientAddon and TogglableClientAddon? Then, when connecting to a server, any TogglableClientAddons that are enabled, but the server does not have, will be disabled until the user leaves the server.

What am I then missing that could cause issues?

Ok, that makes more sense. I had thought that a togglable addon was one that a user could disable at any time, but it makes sense that it'd only be in the menu. I'll make that change.

Second thought, would this mean that we could get rid of the /addon enable and /addon disable commands?

No I'd keep those, because there still is a valid use case for them. If you want to host a server, but are still deciding on which addons you want the server to have, you can enable/disable the addons you want to use.

If that's the goal, then it doesn't work. The addons are only disabled on the client, not the server. That means that players trying to connect will need every addon installed anyways. The host will get a message saying Server requires the following addons: <addon> (disabled), and people attempting to join will get Server requires the following addons: <addon> (missing). What would you recommend?

Liparakis · 2026-08-04T23:12:25Z

+    /// <inheritdoc cref="_disabled" />
+    public bool Disabled {
+        get => _disabled;
+        internal set {
+            var valueChanged = _disabled != value;
+
+            _disabled = value;
+
+            if (!valueChanged) {
+                return;
+            }
+
+            if (value) {
+                try {
+                    OnDisable();
+                } catch (Exception e) {
+                    Logger.Error($"Exception was thrown while calling OnDisable for addon '{GetName()}':\n{e}");
+                }
+            } else {
+                try {
+                    OnEnable();
+                } catch (Exception e) {
+                    Logger.Error($"Exception was thrown while calling OnEnable for addon '{GetName()}':\n{e}");
+                }
+            }


This could be replaced with this, avoiding the double try-catch clauses and making it easier to read.

public bool Disabled { get => _disabled; internal set { var valueChanged = _disabled != value; _disabled = value; if (!valueChanged) { return; } var callbackName = value ? "OnDisable" : "OnEnable"; try { if (value) { OnDisable(); } else { OnEnable(); } } catch (Exception e) { Logger.Error($"Exception was thrown while calling {callbackName} for addon '{GetName()}':\n{e}"); } } }

Liparakis

I've taken a look as-well out of curiosity and i've left some comments 😊

Liparakis · 2026-08-04T23:23:40Z

    /// <summary>
    /// Try to enable the addon with the given name.
    /// </summary>
    /// <param name="addonName">The name of the addon to enable.</param>
-    /// <returns>True if the addon with the given name was enabled; otherwise false.</returns>
+    /// <returns>True if the addon with the given name was enabled, null if it was enabled locally but is disabled by the server, otherwise false.</returns>
    public bool TryEnableAddon(string addonName) {
        foreach (var addon in _addons) {
            if (addon.GetName() == addonName) {
                if (addon is not TogglableClientAddon togglableAddon) {
                    return false;
                }

                togglableAddon.Disabled = false;

                _modSettings.DisabledAddons.Remove(addon.GetName());
                _modSettings.Save();
-
-                return true;
            }
        }

        return false;
    }


The successful return was removed, meaning the method always eventually returns false even after enabling an addon. The XML documentation is also stale and mentions null although the method returns bool!

Additionally you could use a guard clause to avoid nesting.

Liparakis · 2026-08-04T23:42:34Z

+        _addonManager.ToggleServerAllowedAddons(serverInfo.AddonsToDisable);
+
+        if (serverInfo.AddonsToDisable.Length > 0) {
+            UiManager.InternalChatBox.AddMessage($"Disabled {serverInfo.AddonsToDisable.Length} incompatable addons:");


Minor spelling issue: Incompatable -> Incompatible. Also i would add the word "Temporarily" -- it communicates better the intent.

Liparakis · 2026-08-04T23:45:21Z

+    /// <summary>
+    /// Disables all addons not allowed by the server.
+    /// </summary>
+    /// <param name="addonsToDisable">The names of addons to disable.</param>
+    public void ToggleServerAllowedAddons(string[] addonsToDisable) {
+        var addons = addonsToDisable.ToHashSet();
+
+        foreach (var addon in RegisteredAddons) {
+            var isDisabled = addons.Contains(addon.GetName());
+
+            if (addon is TogglableClientAddon optionalAddon) {
+                optionalAddon.Disabled = isDisabled;
+            }
+        }
+    }
+


Could this be renamed to SetServerDisabledAddons or something equivalent? ToggleServerAllowedAddons implies the input contains allowed addons but the parameter contains addons that must be disabled.

Liparakis · 2026-08-05T00:03:21Z

-        // If there is a mismatch between the number of networked addons of the client and the server,
-        // we can immediately invalidate the request
-        if (addonData.Count != AddonManager.GetNetworkedAddonData().Count) {
-            Logger.Debug("  Client addons are invalid, rejected connection");
-
-            HandleInvalidLoginAddons(serverInfo);
-            return;
-        }
-
        // Create a byte list denoting the order of the addons on the server
        var addonOrder = new List<byte>();

+        var disabledAddons = new List<string>();
+
        foreach (var addon in addonData) {
            // Check and retrieve the server addon with the same name and version
            if (!AddonManager.TryGetNetworkedAddon(
                    addon.Identifier,
                    addon.Version,
                    out var correspondingServerAddon
                )) {
+                if (addon.CanBeDisabled) {
+                    disabledAddons.Add(addon.Identifier);
+                    continue;
+                }
                Logger.Debug("  Client addons are invalid, rejected connection");

                // There was no corresponding server addon, so we send a login response with an invalid status
                // and the addon data that is present on the server, so the client knows what is invalid
                HandleInvalidLoginAddons(serverInfo);
                return;
            }

            if (!correspondingServerAddon.Id.HasValue) {
                continue;
            }

            // If the addon is also present on the server, we append the addon order with the correct index
            addonOrder.Add(correspondingServerAddon.Id.Value);
        }


Removing the count equality check makes sense so extra toggleable client addons can be disabled instead of rejecting the connection. However the replacement validation only iterates over the client's addons, so it no longer verifies that every required server addon exists on the client.

For example, a server with addons X and Y appears able to accept a client that only reports X. The loop validates X and then accepts the connection without noticing that Y is missing.

This might require a bit more plumbing but I think we should still verify that all server addons have been matched. Extra toggleable client addons should be allowed, while clients missing required server addons should still be rejected.

Liparakis · 2026-08-05T00:20:49Z

    /// <summary>
    /// Internal logic for disconnecting from the server.
    /// </summary>
    private void InternalDisconnect() {
        Logger.Info("Disconnecting from server");

        _autoConnect = false;

        _netClient.Disconnect();

        // Leave Steam Lobby in case we are connected
        if (SteamManager.IsInLobby) {
            Logger.Info("Leaving Steam lobby.");
            SteamManager.LeaveLobby();
        }

        // Let the player manager know we disconnected
        _playerManager.OnDisconnect();

        // Clear the player data dictionary
        Logger.Info($"Clearing {_playerData.Count} player(s) from store");
        _playerData.Clear();

        _uiManager.OnClientDisconnect();

        _addonManager.ClearNetworkedAddonIds();

        // Check whether the game is in the pause menu and reset timescale to 0 in that case
        if (UIManager.instance.uiState.Equals(UIState.PAUSED)) {
            PauseManager.SetTimeScale(0);
        }

        // Deregister the hooks and handlers
        DeregisterHooks();
        DeregisterPacketHandlers();

        try {
            DisconnectEvent?.Invoke();
        } catch (Exception e) {
            Logger.Warn(
                $"Exception thrown while invoking Disconnect event:\n{e}"
            );
        }
    }


Correct me if I'm wrong, but the current disconnect logic does NOT clear the server-applied disabled state of addons. As a result, a player may leave one server and retain server-specific disabled addons when connecting to another.

This is currently masked by the separate issue where clients missing required addons may still be accepted, but both problems should be fixed independently.

/// <summary> /// Clear all temporary addon restrictions applied by the server. /// </summary> public void ClearServerDisabledAddons() { foreach (var addon in RegisteredAddons) { if (addon is TogglableClientAddon togglableAddon) { togglableAddon.Disabled = false; } } }

Then invoke it alongside _addonManager.ClearNetworkedAddonIds(); during disconnect cleanup.

Make some client addons optional if server addon not present

29ed4f2

Extremelyd1 requested changes Aug 2, 2026

View reviewed changes

simplify to only use TogglableAddon

df6ed42

Liparakis reviewed Aug 4, 2026

View reviewed changes

Liparakis reviewed Aug 5, 2026

View reviewed changes

Uh oh!

Conversation

BobbyTheCatfish commented Aug 1, 2026

Uh oh!

Extremelyd1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Liparakis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants