Yellow-Dog-Man / Resonite-Issues

Issue repository for Resonite.
https://resonite.com
111 stars 0 forks source link

[Headless] Sometimes two UndoManagers are created in the world #490

Open Nytra opened 7 months ago

Nytra commented 7 months ago

Describe the bug?

Sometimes when you start a headless session and the first user joins the session, two Undo Manager slots can be found in the world. Both slots contain a UndoManager component but only one of them works as far as I can tell. Also the first joining user will not be able to properly use undo until they respawn.

One of the UndoManagers is attached by the headless host, the other is attached by the first joining user. I think something in the joining user's client is creating the second UndoManager in the world.

To Reproduce

This is not something that can be consistently replicated. I would say it happens 50% of the time. Maybe less.

1) Start a headless session. 2) Join it. 3) Sometimes there will be two UndoManagers.

Expected behavior

Only one UndoManager is created in the world.

Screenshots

2023-10-31 04 37 16

2023-10-31 04 37 46

Resonite Version Number

Beta 2023.10.20.831

What Platforms does this occur on?

Windows, Linux

What headset if any do you use?

Valve Index, Desktop

Log Files

Headless log file: DESKTOP-H976HO2 - 2023.10.20.831 - 2023-10-31 04_33_30.log

Client log file: DESKTOP-H976HO2 - 2023.10.20.831 - 2023-10-31 01_12_15.log

Additional Context

N/a

Reporters

Nytra (Discord: nytra)

shiftyscales commented 3 months ago

Seeking feedback from @ProbablePrime or @Frooxius - this issue seems to have as much as it can otherwise.

ProbablePrime commented 2 months ago

Took a brief look.

UndoManager is mostly accessed through a helper method that's designed to ensure that only one exists. That helper method scans the hierarchy looking for an existing UndoManager.

Due to this, and the "sometimes" here I think we're probably looking at some form of race condition.

Getting any way to increase the likelihood of this happening would help tremendously.

Banane9 commented 2 months ago

Due to this, and the "sometimes" here I think we're probably looking at some form of race condition.

I would agree. On the client's side, it would seem the initialization of the InteractionHandler triggers the creation of the UndoManager - when does the headless create it? Perhaps the easiest solution would be to ensure that the headless always creates the UndoManager as soon as a world is loaded - which would be equivalent to what regular client hosted worlds do, as it's immediately created when the host joins.

Of course it would sense to not rely on the host doing it there either, perhaps explicitly adding it to the world loading / preparation for both.

shiftyscales commented 2 months ago

On a related note- is there a particular reason why the Undo Manager itself is marked non-persistent in the first place, @ProbablePrime? I assume it's to clear all undo steps between sessions- but the child slots created by it are marked persistent- that seems a bit backwards.

Checking if an undo manager isn't already present in the world seems like something that should probably be done when it's explicitly missing rather than every single session?

PJB3005 commented 2 months ago

but the child slots created by it are marked persistent- that seems a bit backwards.

Doesn't making a parent slot non-persistent apply to all descendants too, regardless of whether they are marked persistent themselves or not?

shiftyscales commented 2 months ago

Yes- anything that is parented under a non-persistent slot is also implicitly non-persistent, @PJB3005.

ProbablePrime commented 2 months ago

The persistence setup is fine, with the top level (Undo Manager) not being persistent the created undo steps are not saved.

Let's focus on the duplicate undo managers to which I'll have another look.

ProbablePrime commented 2 months ago

In cases where two undo managers exist, could you check the contents of the Root slot for an additional undomanager?

Nytra commented 2 months ago

Not sure what you meant by this exactly, but I had a case with two UndoManagers, then I checked the components on the Root slot and found no UndoManager in there. Using a find child by name it could only find the two "Undo Manager" slots that were direct children of the Root slot.

Nytra commented 2 months ago

Oh there is something weird happening here though.

I have loaded up a headless and in this case there is only one UndoManager visible in the Root slot through the inspector, and it has no children.

However I have managed to get a reference to a second one (using find child by name) which has children and seems to be the one which is actively in use by me, although it does not appear to be visible in the Root slot. It is somehow hidden.

This might be an UndoManager that only exists locally for me and not for the headless host

2024-03-12 07 31 04

Nytra commented 2 months ago

This appears to be fairly consistent. I think whenever the first user joins a headless session it always makes two undo managers, just you can't always see both of them in the host's inspector. I believe you can test for this by joining a headless session and start grabbing things then check if the Undo Manager slot in Root visible via inspector has any children or not. If it has no children it means you are using a local Undo Manager.

ProbablePrime commented 2 months ago

This is probably the same thing as the other bug.

here's a log snippet:

00:24:19.503 ( 42 FPS)  Restoring parent of slot Undo Managernew parent: Root (ID2400) <T: [0; 0; 0], R: [0; 0; 0; 1], S: [1; 1; 1], IsActive: True, ActiveSelf: True, Started: False, Destroyed: False, Disposed: False, Local: False> Parent: <null>
00:24:19.503 ( 42 FPS)  NewParentAvailable. For: Undo Manager (ID294F00), Parent: Root (ID2400), currentParent:  ()

  at System.Environment.get_StackTrace () [0x00000] in <9577ac7a62ef43179789031239ba8798>:0 
  at Elements.Core.UniLog.Log (System.String message, System.Boolean stackTrace) [0x00000] in <31ec75cb614d475f98ab0e55ccf6696e>:0 
  at FrooxEngine.Slot.NewParentSlotAvailable (FrooxEngine.SyncRef`1[T] reference) [0x00000] in 

same protection logic triggering and doing funny parent stuff.

ProbablePrime commented 2 months ago

UndoManager is protected, Yeah this is the same bug wow.

Frooxius commented 2 months ago

Merged this fix in 2024.3.12.1169, thanks!

Nytra commented 2 months ago

I don't think this is fixed. It actually seems to happen much more consistently now, like every single time you are the first user to join a headless session.

Logs:

graphical client - DESKTOP-H976HO2 - 2024.3.12.1169 - 2024-03-12 19_57_23.log

headless client - DESKTOP-H976HO2 - 2024.3.12.1169 - 2024-03-12 19_59_36.log

shiftyscales commented 2 months ago

Well- if it consistently happens now- that is at least an improvement towards finding the underlying cause.

ProbablePrime commented 2 months ago

Thanks for the Fresh logs, this will be a lot easier to work on with the other bug out of the way.

ProbablePrime commented 2 months ago

@Nytra Are you able to reproduce this without a headless?

ProbablePrime commented 2 months ago

For added enjoyment, play this in the background while reading this.


Remember to read part 1: https://github.com/Yellow-Dog-Man/Resonite-Issues/issues/490#issuecomment-1980083328

So apparently exactly 6 hours ago(I haven't been working on it that long I promise, I've had a dog walk and dinner since then :D) I started working on this again as my first step was to figure out why this only appeared to happen on a headless.

I can't seem to keep a stable headless setup on my main development machine, mostly because of the cruft that accumulates when you're building and managing multiple versions of Resonite. It's always broken when I go to use it.

So, because I was frustrated I wrote:

and then followed my own guidance on those pages to run a headless.

With that operational I got back to the actual bug, I just added a logging line that prints the stack trace everytime something tries to get the Undo Manager in the world, then joined the headless. Boom 2 Undo Managers first time. But in my haste to check if there were two undo managers I realized I had .... while the world was loading.... equipped a tool. Equipping a tool is an undo-able operation, that requires.... the undo manager.

So my thoughts go to a race condition. I'm trying to register the undo manager because I'm equipping the tool and then something in the headless is then register the undo manager later. If they conflict or fight somehow in a race condition manner then there's your issue 2 Undo Managers:

So then I get to thinking why doesn't this bug occur with Regular User based sessions. Looking at the same logfile, I notice that one of the first things an avatar does is to setup its InteractionHandler, which requires getting the undo manager:

03:31:01.310 (  0 FPS)  Get Undo manager, True

  at System.Environment.get_StackTrace () [0x00000] in <9577ac7a62ef43179789031239ba8798>:0 
  at Elements.Core.UniLog.Log (System.String message, System.Boolean stackTrace) [0x00000] in <ebcf2cba0793432d9877a63b284d682f>:0 
  at FrooxEngine.Undo.UndoManagerExtensions.GetUndoManager (FrooxEngine.World world, System.Boolean addNew) [0x00000] in <add1221686e04bed8d7d9df3072b0daf>:0 
  at FrooxEngine.InteractionHandler.OnStart () [0x00000] in <add1221686e04bed8d7d9df3072b0daf>:0 

And as headless users, in addition to not having heads, do not also have hands, it means they don't have InteractionHandlers.

So in summary, as to why this only works on headless sessions:

But, why does the interaction handler need the UndoManager?

So it can attach button event handlers... toooo the context menus!! image

And guess what headless users don't have... Context Menus!

Anyway, Tomorrow, I'm going to see what I can do just make part of the world setup include an undo manager. It'll basically be:

Headless OnStart {
   GetTheUndoManager 
}

That way an undo manager will exist before a regular user can join.

It looks like the code was originally added to only setup an undo manager when the system was used in some way. This would make static worlds like the Meta Movie project world NOT load undo managers. But as everyone has those context menu items it makes sense to make this component always exist.

With the settings rework and Headless Configuration files, in worlds that don't change we can probably expose a setting or configuration thingy to turn off the undo manager if needed.

JackTheFoxOtter commented 2 months ago

With the settings rework and Headless Configuration files, in worlds that don't change we can probably expose a setting or configuration thingy to turn off the undo manager if needed.

This would be a very appreciated feature coming out of this rabbit hole. (Was interesting to follow you dig into this though!). There are situations, especially for game worlds, where the Undo Manager isn't really wanted, and its precense requires engineering solutions around it to ensure players can't break the game using undo / redo. There is #284 and #285 which are related to customizing undo functionality. (I'm aware these are for disabling undo functionality and not the entire undo manager, but I wanted to link them anyway as they likely touch similar systems).

ProbablePrime commented 2 months ago

Ok jumped back on this to take a look.

Did some changes and.... now there are 3 Undo Managers in the world :D

Banane9 commented 2 months ago

Did some changes and.... now there are 3 Undo Managers in the world :D

Perfect, now just do the exact opposite of those changes and this should be fixed 😁