Fixes on instance information/destruction #3141
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
also do some style fix to instance functions
guilherme-gm
force-pushed
the
fix-instance-destroy
branch
from
71e7472 to
9c6d0b8
Compare
4144
requested changes
May 11, 2022
guilherme-gm
force-pushed
the
fix-instance-destroy
branch
from
9c6d0b8 to
b89e3e4
Compare
MrKeiKun
reviewed
May 12, 2022
- Properly clean up freed instance data to prevent link by partial info. Instance state should always be checked, but in case it is forgotten, it would lead to saying a player has an instance that may not even exist - Mark instance being destroyed to stop sending info to player after timeout. This would happen when an instance expire and players are kicked out of it, as leaving the instance would be counted as making it idle - Adds check for active instance on player login. Otherwise player may receive instance window about an instance that doesn't exists anymore
Destroyed instances (invalid) are kept in memory for reuse, but they should not be reinitialized during a reload. Just keep its memory space as is.
Otherwise, hercules will try to free it again during shutdown and fail
in some cases, alive and idle timeout could get mixed and the wrong message would be sent to the user.
guilherme-gm
force-pushed
the
fix-instance-destroy
branch
from
b89e3e4 to
37bca6a
Compare
MishimaHaruna
approved these changes
Jun 2, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Prelude
Changes Proposed
This PR fixes some issues around instance information and destruction, that could cause visual glitches and assertion failures. Also, I have changed the response numbers into an enum to make it clear. Those are the issues addressed:
Issue 1: When players were moved out of an instance due to it being expired, they were receiving the information that the instance actually went Idle instead of destroyed. That was caused because when moving players out of the instance, it is still considered a valid instance, and this triggers the idle check.
To fix that, instances are now marked as "being destroyed", and thus invalid, before moving players out of it.
Issue 2: When a player had a Player Instance attached to them, even after being destroyed, they were still receiving the instance info when logging back in. That was caused because the check for player instance on login was not taking into account if the instance was freed or not (via
.state).To fix that, instance data is now fully cleaned up upon destruction (using
memsetinstead of only zeroing a few fields) and the state check was added to the login process.memsetstep is not necessarily required, but it would avoid future issues if someone forget to check the state.Issue 3: An assert failure when reloading scripts after playing an instance. That was caused due to instance reload trying to recreate a freed instance.
Issue 4: An assert failure when shutting the server down after playing an instance. That was caused due to hercules trying to free the already freed instance map.
Issue 5: In some cases, the wrong reason code was provided when an instance gets destroyed. It now checks for instance idle state to help choosing the proper reason.
Issues addressed: None, I think