Fix SSD text delay on examine #39241

path-to-aaru · 2025-07-27T11:38:38Z

About the PR

This PR refactors the way mind status text on examine is handled to use a client-predicted component.

Why / Balance

Removes the delay on SSD/death text populating when examining entities that is caused by waiting for the server to provide mind status information. This fix was asked for in the SharedMindSystem code by a TODO comment. See Media for comparison.

Technical details

Mind status OnExamine logic removed from SharedMindSystem and handled by a new component and systems - MindStatusComponent, MindExamineSystem, and MindStatusSystem. Tested on localhost and no issues found, but you never know. Might contain some strange or unnecessary code, I'm coming from a C++ background, so feel free to ask for revisions.

Media

mind-status-examine0001-0563.mp4

Requirements

I have read and am following the Pull Request and Changelog Guidelines.
I have added media to this PR or it does not require an ingame showcase.

Breaking changes

Content.Shared.SharedMindSystem.cs - Removed OnExamine method and refactored into MindStatusSystem.cs

Changelog
🆑

fix: Fixed SSD/death examine text not being predicted

perryprog

I'm not very familiar with mind code so I can't comment much on the approach, but I am pretty confident that it would be better to avoid using an update loop here: it's possible this will mean you need to add some events to MindSystem for this.

perryprog · 2025-07-27T16:37:12Z

Content.Shared/Mind/MindExamineSystem.cs

+        SubscribeLocalEvent<MindStatusComponent, ExaminedEvent>(OnExamined);
+    }
+
+    private void OnExamined(EntityUid uid, MindStatusComponent component, ExaminedEvent args)


In general using the Entity<T> variant of event handlers is preferred—here the signature would be OnExamined(Entity<MindStatusComponent> ent, ref ExaminedEvent args).

perryprog · 2025-07-27T16:40:36Z

Content.Shared/Mind/MindStatusSystem.cs

+    [Dependency] private readonly INetManager _net = default!;
+
+    private TimeSpan _nextUpdate = TimeSpan.Zero;
+    private const float UpdateInterval = 1.0f;


These should be part of the component to ensure it gets serialized correctly, and the update interval should be a TimeSpan. I'm also not positive that this needs an update loop at all—it'd be better if this is something that could just be updated in response to events. You'd then want to make _nextUpdate is an [AutoPausedField], and for it to get set to CurTime on MapInit.

Yeah I agree this sussed me out - I think the solution is to just not do it at all because even having it on the component is a little odd. Events is right way I think

perryprog · 2025-07-27T16:42:50Z

Content.Shared/Mind/MindStatusSystem.cs

+        SubscribeLocalEvent<MindContainerComponent, MindRemovedMessage>(OnMindRemoved);
+        SubscribeLocalEvent<ForceUpdateMindStatusEvent>(OnForceUpdate);
+
+        if (_net.IsServer)


Instead of having all these _net.IsClient / _net.IsServer, it'd probably be better to 1) make MindStatusSystem abstract and rename it to SharedMindStatusSystem, 2) in both Content.Client and Content.Server, create their own MindStatusSystems which inherit from SharedMindStatusSystem, and then 3) keep all the server-specific code in the version that's in Content.Server.

That being said, I'm not sure if this is needed, as it looks like nearly all of these events are only raised by the server, meaning you don't need the checks at all—it just won't ever run on the client.

perryprog · 2025-07-27T16:44:37Z

Content.Shared/Mind/MindStatusSystem.cs

+        if (_timing.CurTime < _nextUpdate) // Periodic update for edge cases
+            return;
+
+        _nextUpdate = _timing.CurTime + TimeSpan.FromSeconds(UpdateInterval);


Even though precision is not important here, it's good practice to increment via _nextUpdate += UpdateInterval. Otherwise each time between updates will be slightly longer than the UpdateInterval, since Update isn't called every single (arbitrary small unit of time).

perryprog · 2025-07-27T16:46:51Z

Content.Shared/Mind/MindStatusSystem.cs

+        }
+    }
+
+    private void OnStartup(EntityUid uid, MindStatusComponent component, ComponentStartup args)


All of these should use the Entity<T> form as mentioned earlier.

perryprog · 2025-07-27T17:11:26Z

Content.Shared/Mind/MindStatusSystem.cs

+    {
+        var oldStatus = status.Status;
+        var dead = _mobState.IsDead(uid);
+        var mind = container.Mind != null ? CompOrNull<MindComponent>(container.Mind) : null;


Suggested change

var mind = container.Mind != null ? CompOrNull<MindComponent>(container.Mind) : null;

var mind = CompOrNull<MindComponent>(container.Mind);

perryprog · 2025-07-27T17:12:45Z

Content.Shared/Mind/MindStatusSystem.cs

+        var hasActiveSession = hasUserId != null && _playerManager.ValidSessionId(hasUserId.Value);
+
+        // Determine new status based on the scenarios
+        if (dead && hasUserId == null)


If you wanted to be super funky you could make this a switch expression over dead, hasUserId, and hasActiveSession and pattern match everything. (This is fine though.)

perryprog · 2025-07-27T17:14:48Z

Content.Shared/SSDIndicator/SSDIndicatorSystem.cs

    [Dependency] private readonly IConfigurationManager _cfg = default!;
    [Dependency] private readonly IGameTiming _timing = default!;
    [Dependency] private readonly StatusEffectsSystem _statusEffects = default!;
+    [Dependency] private readonly EntityManager _entityManager = default!;


This is already available as an EntitySystem field as just EntityManager.

perryprog · 2025-07-27T17:16:06Z

Content.Shared/Mind/Components/MindStatusComponent.cs

+public sealed class ForceUpdateMindStatusEvent : EntityEventArgs
+{
+    public EntityUid Entity { get; }
+
+    public ForceUpdateMindStatusEvent(EntityUid entity)
+    {
+        Entity = entity;
+    }
+}


Structs are preferred:

Suggested change

public sealed class ForceUpdateMindStatusEvent : EntityEventArgs

{

public EntityUid Entity { get; }

public ForceUpdateMindStatusEvent(EntityUid entity)

{

Entity = entity;

}

}

[ByRefEvent]

public readonly record struct ForceUpdateMindStatusEvent(EntityUid Entity);

perryprog · 2025-07-27T17:16:42Z

Content.Shared/SSDIndicator/SSDIndicatorSystem.cs

        }

        Dirty(uid, component);
+        _entityManager.EventBus.RaiseEvent(EventSource.Local, new ForceUpdateMindStatusEvent(uid));


Same for other, but this should be:

Suggested change

_entityManager.EventBus.RaiseEvent(EventSource.Local, new ForceUpdateMindStatusEvent(uid));

var ev = new ForceUpdateMindStatusEvent(uid);

RaiseLocalEvent(uid, ref ev);

beck-thompson

I'm not sure this is the correct approach 🤔
Its pretty complex and I worry it'll eventually break

beck-thompson · 2025-07-27T17:11:25Z

Content.Shared/Mind/Components/MindStatusComponent.cs

+namespace Content.Shared.Mind.Components;
+
+[RegisterComponent, NetworkedComponent, AutoGenerateComponentState]
+public sealed partial class MindStatusComponent : Component


Add comment

beck-thompson · 2025-07-27T17:11:37Z

Content.Shared/Mind/Components/MindStatusComponent.cs

+    DeadAndIrrecoverable
+}
+
+public sealed class ForceUpdateMindStatusEvent : EntityEventArgs


beck-thompson · 2025-07-27T17:12:10Z

Content.Shared/Mind/MindExamineSystem.cs

+            MindStatus.Catatonic => Loc.GetString("comp-mind-examined-catatonic", ("ent", uid)),
+            MindStatus.SSD => Loc.GetString("comp-mind-examined-ssd", ("ent", uid)),
+            MindStatus.Active => null, // No special text for active players
+            _ => null


Throw an error here instead

beck-thompson · 2025-07-27T17:12:42Z

Content.Shared/Mind/MindExamineSystem.cs

+        }
+    }
+
+    private static string GetColorForStatus(MindStatus status)


I think it makes more sense to put this with the LOC strings themselves

beck-thompson · 2025-07-27T17:14:51Z

Content.Shared/Mind/MindStatusSystem.cs

+        }
+    }
+
+    public override void Update(float frameTime)


Put the update at the bottom of all the functions!

Also this function gives me sus vibes - does the system not work without it?

beck-thompson · 2025-07-27T17:17:15Z

Content.Shared/Mind/MindStatusSystem.cs

+        SubscribeLocalEvent<MindContainerComponent, MindRemovedMessage>(OnMindRemoved);
+        SubscribeLocalEvent<ForceUpdateMindStatusEvent>(OnForceUpdate);
+
+        if (_net.IsServer)


In 90% of situations you shouldn't be using IsClient / IsServer, its usually best to just split the function into a server and client version. Do it like this:

In Shared make a SharedMindStatusSystem that is abstract. Put all subscriptions and functions that need to run in both client and shared there.

In Server make a MindStatusSystem that is NOT abstract. Extend the shared abstract system and then add any new functions subscriptions, etc... that can't be run in the client.

Repeat for Client but stuff that only the client can run not the server

beck-thompson · 2025-07-27T17:28:41Z

Content.Shared/SSDIndicator/SSDIndicatorSystem.cs

        }

        Dirty(uid, component);
+        _entityManager.EventBus.RaiseEvent(EventSource.Local, new ForceUpdateMindStatusEvent(uid));


Use RaiseLocalEvent

beck-thompson · 2025-07-27T17:41:36Z

I'm not very familiar with mind code so I can't comment much on the approach, but I am pretty confident that it would be better to avoid using an update loop here: it's possible this will mean you need to add some events to MindSystem for this.

Welp perry you sniped me... Double review double the fun I guess 😆

I wish github would give a big warning like: WARNING SOMEONE POSTED A REVIEW WHILE YOU WERE MAKING COMMENTS!!

path-to-aaru · 2025-07-27T22:10:33Z

Huge thanks for the detailed review, a lot of stuff to unpack but already a big help in understanding Robust Toolbox practices, I really appreciate it. I had a simpler solution that I liked a lot more but it kept saying that I had a duplicate subscription that I just couldn't pinpoint so I went for this as a workaround last night and I can see all the problems with it now. Will probably not have another solution as my weekend is over now but I can take another shot at it later if it hasn't been addressed by then. Thanks again folks!

beck-thompson · 2025-07-27T22:26:09Z

Huge thanks for the detailed review, a lot of stuff to unpack but already a big help in understanding Robust Toolbox practices, I really appreciate it. I had a simpler solution that I liked a lot more but it kept saying that I had a duplicate subscription that I just couldn't pinpoint so I went for this as a workaround last night and I can see all the problems with it now. Will probably not have another solution as my weekend is over now but I can take another shot at it later if it hasn't been addressed by then. Thanks again folks!

yeah your PR is solid and def has the right core idea! If you see both of the comments on the same thing, it looks like perry has a more detailed answers so go with those

path-to-aaru · 2025-08-02T06:24:44Z

Huge thanks for the detailed review, a lot of stuff to unpack but already a big help in understanding Robust Toolbox practices, I really appreciate it. I had a simpler solution that I liked a lot more but it kept saying that I had a duplicate subscription that I just couldn't pinpoint so I went for this as a workaround last night and I can see all the problems with it now. Will probably not have another solution as my weekend is over now but I can take another shot at it later if it hasn't been addressed by then. Thanks again folks!

yeah your PR is solid and def has the right core idea! If you see both of the comments on the same thing, it looks like perry has a more detailed answers so go with those

I was able to implement yours and perry's suggestions for the most part, but in diving deeper and attempting to move away from the update loop, I'm noticing some inconsistencies between the current status descriptions (without any of my changes) and when they're actually used:

"Catatonic" status - Current description is "{ent} is totally catatonic. The stresses of life in deep-space must have been too much for {ent}. Any recovery is impossible." When it's used - Living entities that have never had a mind (admin-spawned mobs, test dummies on test maps). The description says "recovery is impossible", but if these entities are often specifically spawned to be possessed by ghosts, players might examine them, see "recovery is impossible", then moments later see them walking around controlled by a player.

Then there's the "Dead and Irrecoverable" status. Current description is "{ent} soul has departed and moved on. Any recovery is impossible." The issue is that it seems like this should apply to players who have died and rotted beyond revival, but an unrevivable player corpse still just shows as the "dead" or "dead and SSD" statuses rather than "dead and irrecoverable", even if they are truly irrecoverable at that point. I'm unable to determine when exactly "Dead and Irrecoverable" ever happens in its current state.

I'm able to match the functionality of the current system (but with predicted text) using an update loop, but it's difficult to move to an event-driven update system because the actual application of these different statuses seems to have some issues. Or I'm just misunderstanding the intentions, which is totally possible! Can you clarify the intention of the "Catatonic" and "Dead and Irrecoverable" descriptions?

Refactor mind status on examine to use client-predicted component

85ba922

PJBot added S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Jul 27, 2025

github-actions bot added the size/M Denotes a PR that changes 100-999 lines. label Jul 27, 2025

perryprog reviewed Jul 27, 2025

View reviewed changes

beck-thompson requested changes Jul 27, 2025

View reviewed changes

PJBot added S: Awaiting Changes Status: Changes are required before another review can happen and removed S: Needs Review Status: Requires additional reviews before being fully accepted. Not to be replaced by S: Approved. labels Jul 27, 2025

path-to-aaru closed this Aug 11, 2025

	var mind = container.Mind != null ? CompOrNull<MindComponent>(container.Mind) : null;
	var mind = CompOrNull<MindComponent>(container.Mind);

	_entityManager.EventBus.RaiseEvent(EventSource.Local, new ForceUpdateMindStatusEvent(uid));
	var ev = new ForceUpdateMindStatusEvent(uid);
	RaiseLocalEvent(uid, ref ev);

Fix SSD text delay on examine #39241

Fix SSD text delay on examine #39241

Uh oh!

Conversation

path-to-aaru commented Jul 27, 2025

About the PR

Why / Balance

Technical details

Media

Requirements

Breaking changes

Uh oh!

perryprog 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!

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!

beck-thompson 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beck-thompson commented Jul 27, 2025

Uh oh!

path-to-aaru commented Jul 27, 2025

Uh oh!

beck-thompson commented Jul 27, 2025

Uh oh!

path-to-aaru commented Aug 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants