Bugfix: SharedUserInterfaceSystem stores and applies BUI states in order.

[whatston3]

What the title says.

Related issues

• PDA Cartridges failing to load more often than usual, lately. space-station-14#44758 (needs engine update to resolve)

From the solution in #6789, instead of using the most recent state in every update, we cache the intermediate states (appending a third element into the tuple) and update with each intermediate value when predicting. In addition, all open operations are only opens.

This is particularly naive, and I don't know what lies in between the state population and that Update call.

There's also a lingering BUI.UpdateState/Update call in UISystem.SetUiState that should possibly be deferred.

I hate that the list Remove call uses an explicit null value, but all open/close additions should be null.

Small video of the PDA working with this change:

pda-fix.mp4

[eoineoineoin]

Looks like this new bug was my fault - whoops. I'm a little concerned about this, though:

In addition, all open operations are only opens.

Even before #6789, the open operations were doing a bui.Open(), bui.UpdateState() and bui.Update(), hence the slightly awkward conditions. When I was making that change, it seemed pretty important to preserve that behaviour - there are ComponentStartup and PlayerAttached events which will try to open UIs, but look like they're not going to get an UpdateState message, so skipping the updates may break UIs in those cases.

[whatston3]

Even before #6789, the open operations were doing a bui.Open(), bui.UpdateState() and bui.Update(), hence the slightly awkward conditions. When I was making that change, it seemed pretty important to preserve that behaviour - there are ComponentStartup and PlayerAttached events which will try to open UIs, but look like they're not going to get an UpdateState message, so skipping the updates may break UIs in those cases.

The thing is that you break the sequence of state updates if you give it the latest state, and as far as I know, you don't conveniently have the state at the time. If that's easy to get (is it inside of the UIComponent for the relevant key?) then it could be shoved into the Open [command when it's queued], no problem, but you probably should not break the sequence of state updates as they happen (hence the concerns w.r.t. UISystem.SetUiState)

[whatston3]

This feels much worse than before #6789. Closing for now and recommending to revert #6789.

Might look into underlying issues more, but this is a dead-end unless I'm missing something.

[whatston3]

After a handful of tests, it seems to behave the same as it did before #6789 (good lord).

Reopening this.

[Princess-Cheeseballs]

Remind me to review tonight and get it mergeable so we can do a patch for the current engine version

[deltanedas]

tested, works

[Princess-Cheeseballs]

Code makes sense. At worst I could nitpick the dictionary method being TryGetValue instead of GetValueOrDefault but It's not a big deal at all.

[whatston3]

Code makes sense. At worst I could nitpick the dictionary method being TryGetValue instead of GetValueOrDefault but It's not a big deal at all.

Should be changed.

[deltanedas]

pretty sure this broke gas filter ui and cargo shuttle

if you open gas filter it wont update anything until you e.g. change gas
i imagine its because of using before activatable ui event for setting ui state

[whatston3]

pretty sure this broke gas filter ui and cargo shuttle

if you open gas filter it wont update anything until you e.g. change gas i imagine its because of using before activatable ui event for setting ui state

Apologies, then. I'll look into it.

[whatston3]

pretty sure this broke gas filter ui and cargo shuttle

if you open gas filter it wont update anything until you e.g. change gas i imagine its because of using before activatable ui event for setting ui state

you're gonna love the fix for this one

[eoineoineoin]

Is everyone happy with this solution and the associated hotfix?

I did a little digging, as the claim "we cache the intermediate states" seemed kinda funny to me, and I didn't see how #6789 would have broken it. What's happening is that the PDABoundUserInterface sends multiple different implementations of BoundUserInterfaceState to update subsets of the UI. But the SharedUserInterfaceSystem is written in a way that assumes a single BoundUserInterfaceState will describe the whole UI. i.e., it contains code like:

Entity<UserInterfaceComponent> ent = ...;
BoundUserInterfaceState buiState = ...
...
ent.Comp.States[key] = buiState;

So, relying on the stored state for some key is not a reliable way to reconstruct the UI. I suspect there's probably a lingering bug here.

If we're not all completely happy with this fix, I'll spend some time this weekend to figure out how this could be improved.