cyberjunk / meridian59-dotnet

New 3D client and tools for Meridian 59
GNU General Public License v3.0
34 stars 35 forks source link

UI didnt unload after trying to reach character select with "quit". #201

Closed MorbusM59 closed 7 years ago

MorbusM59 commented 7 years ago

Also, the character select window wouldn't pop up. Screenshot: bug

Logs: Meridian59.log.txt Ogre.log.txt CEGUI.log.txt

skittles1 commented 7 years ago

More info - seems to only happen when using 'quit' on a live server, but not when doing so on a local server (same Ogre client).

skittles1 commented 7 years ago

Looking into this also, if one of you guys fixes this please increment the Ogre client min version to 11 as I'll update the live client.

cyberjunk commented 7 years ago

That is weird. I can't reproduce it on 105 or 112. However, there is an exception in your Meridian59.log telling something about a parser error on server received data.

cyberjunk commented 7 years ago

This is the network error from your logs:

5:46:55 PM ServerConnection Error Value cannot be null. Parameter name: str PacketDump: 0F-00-F7-C5-0F-00-A5-50-00-00-00-00-00-00-00-00-00-00-00-00-00-00

The packet-type is the 0x50. It seems to be BP_MAIL. Huh, that sounds strange.. However it's also weird that it's only 0x00 in the data parts there. There probably should not be a BP_MAIL?! But a broken message would not kill the client in general.

cyberjunk commented 7 years ago

Here is a messagelog for a successful quit (with Debug Client on 112)

I had to uncomment this line or you would not see the first two messages (log gets reset) Could you may be post a broken one?

cyberjunk commented 7 years ago

You clearly get the BP_QUIT from the server. It's the one that resets the data and reloads the DemoScene. Also you clearly do NOT get the BP_CHARACTERS, as it would be the one switching the UI to AvatarSelection and showing the Welcome window.

skittles1 commented 7 years ago

So there are two issues - in Morbus's screenshot and on the first time I tried to replicate it on 106, we didn't receive the character select box after quitting. I can't reproduce this again and can only reproduce the second part of the bug, which is the UI not disappearing until character select is loaded. This means if someone has 1s latency, the UI will stay up for a second before switching to character select screen.

My delayed char select debug log looks exactly the same as the image you posted. Most users might not notice this (and nobody has reported it yet), though it does stick out over 300ms.

cyberjunk commented 7 years ago

I can't reproduce this again and can only reproduce the second part of the bug, which is the UI not disappearing until character select is loaded.

The UI will be switched to AvatarSelection when the BP_CHARACTERS is received. But the data reset and demoscene loading will be already done on earlier BP_QUIT. Hence the delay with a screen shown by Morbus. There is already a definition for UIMode.None.

I think we just have to use this for this time span.

cyberjunk commented 7 years ago

Have a look here: https://github.com/cyberjunk/meridian59-dotnet/commit/9ef67bb6357e259115353ae115505b49a7aeceaf

The UIMode will now be set to None in DataController Reset().

This will cause no visible UI at all while the mentioned time span between BP_QUIT (->UIMode.None) and BP_CHARACTERS (->UIMode.AvatarSelection)

cyberjunk commented 7 years ago

However, while this 'fixes' the UI, it does not explain why he got stuck and got a parser exception...

cyberjunk commented 7 years ago

I think I found the issue. I can now reproduce the STUCK situation. It happens if the client sends any more messages after BP_REQ_QUIT.

There is technically no kind of block after BP_REQ_QUIT so far. The stuck-log looks like this:

Is it because the server does not unterstand the old GAME mode messages which are received after BP_REQ_QUIT, because he swtiched to LOGIN protocol mode.

cyberjunk commented 7 years ago

The 0x06 answer is AP_RESYNC, due to the unrecognized messages after BP_REQ_QUIT.

cyberjunk commented 7 years ago

Ok, going to fix this on 2 layers.

First: https://github.com/cyberjunk/meridian59-dotnet/commit/8acd143d16a83f0988076d7b4b3511e7d032725c

I think it's a good idea to use the same technique to block further input as used for server-save waiting. Sending BP_REQ_QUIT will now flip IsWaiting. However, the splash-popup-title currently says "SAVING". Did @MorbusM59 change that? Hmm. May be it should be more generally stating 'PLEASE WAIT' now, but 'SAVING' isn't that wrong. But I noticed there are a lot more of 'SendXY()' methods which should actually check for this waiting flag. Only very few (e.g. SendReqMove()) do so far! (TODO)

The second fix will be very low-level, making sure ServerConnection.cs surpresses any message in between BP_REQ_QUIT and BP_QUIT.

cyberjunk commented 7 years ago

And here is the second part: https://github.com/cyberjunk/meridian59-dotnet/commit/b428150aa762c283af1d7a3bba4e3362d4761007

Discards all messages accidentially sent after BP_REQ_QUIT and before receiving BP_QUIT in ServerConnection.

Also has small fixes for the incorrect use of messageController.Mode to determine handler-type. Which really shouldn't be used (e.g. for Quit it does not work, because MessageController switched back to 'LoginMode' while the Quit was still a 'GameMode')

cyberjunk commented 7 years ago

Please let me know if this works smooth now.

MorbusM59 commented 7 years ago

Wow, thanks for all the work and the super quick fix! Will have to test in the evening.

skittles1 commented 7 years ago

Works for me, wondering if we should increment the minor version and release again.

cyberjunk commented 7 years ago

wondering if we should increment the minor version and release again.

I guess that is your decision :) The problem was not really introduced with the last version and existed for a while. But I'm OK with hotfixing it.

skittles1 commented 7 years ago

Fix is live now so can close this one.