Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ghost life cycle edge cases #29

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix ghost life cycle edge cases #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f10a19
Fix ghost life cycle edge cases
May 24, 2022
f80b8f0
Fix GravityHelper issue
Jun 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 42 additions & 53 deletions CelesteNet.Client/Components/CelesteNetMainComponent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,15 +128,7 @@ protected override void Dispose(bool disposing) {
}

public void Cleanup() {
Player = null;
PlayerBody = null;
Session = null;
WasIdle = false;
WasInteractive = false;

foreach (Ghost ghost in Ghosts.Values)
ghost?.RemoveSelf();
Ghosts.Clear();
ResetState();

if (IsGrabbed && Player.StateMachine.State == Player.StFrozen)
Player.StateMachine.State = Player.StNormal;
Expand Down Expand Up @@ -168,8 +160,7 @@ public void Handle(CelesteNetConnection con, DataPlayerInfo player) {
return;

if (string.IsNullOrEmpty(player.DisplayName)) {
ghost.RunOnUpdate(ghost => ghost.NameTag.Name = "");
Ghosts.TryRemove(player.ID, out _);
RemoveGhost(player);
LastFrames.TryRemove(player.ID, out _);
Client.Data.FreeOrder<DataPlayerFrame>(player.ID);
return;
Expand All @@ -193,13 +184,9 @@ public void Handle(CelesteNetConnection con, DataChannelMove move) {
}

} else {
if (!Ghosts.TryGetValue(move.Player.ID, out Ghost ghost) ||
ghost == null)
if (!RemoveGhost(move.Player))
return;

ghost.RunOnUpdate(ghost => ghost.NameTag.Name = "");
Ghosts.TryRemove(move.Player.ID, out _);

foreach (DataType data in Client.Data.GetBoundRefs(move.Player))
if (data.TryGet(Client.Data, out MetaPlayerPrivateState state))
Client.Data.FreeBoundRef(data);
Expand All @@ -221,8 +208,7 @@ public void Handle(CelesteNetConnection con, DataPlayerState state) {

Session session = Session;
if (session != null && (state.SID != session.Area.SID || state.Mode != session.Area.Mode || state.Level == LevelDebugMap)) {
ghost.RunOnUpdate(ghost => ghost.NameTag.Name = "");
Ghosts.TryRemove(id, out _);
RemoveGhost(state.Player); // If we get here, id must belong to a valid ghost, so it can't be uint.MaxValue and state.Player mustn't be null
return;
}

Expand Down Expand Up @@ -597,9 +583,11 @@ protected Ghost CreateGhost(Level level, DataPlayerInfo player, DataPlayerGraphi
return ghost;
}

protected void RemoveGhost(DataPlayerInfo info) {
Ghosts.TryRemove(info.ID, out Ghost ghost);
protected bool RemoveGhost(DataPlayerInfo player) {
if (!Ghosts.TryRemove(player.ID, out Ghost ghost))
return false;
ghost?.RunOnUpdate(g => g.NameTag.Name = "");
return true;
}

public void UpdateIdleTag(Entity target, ref GhostEmote idleTag, bool idle) {
Expand Down Expand Up @@ -670,25 +658,18 @@ public override void Update(GameTime gameTime) {
GrabbedBy = null;

if (ready && Engine.Scene is MapEditor) {
Player = null;
PlayerBody = null;
Session = null;
WasIdle = false;
WasInteractive = false;
ResetState();
AreaKey area = (AreaKey) f_MapEditor_area.GetValue(null);

if (MapEditorArea == null || MapEditorArea.Value.SID != area.SID || MapEditorArea.Value.Mode != area.Mode) {
MapEditorArea = area;
// FIXME: NOTE BEFORE MERGING: can we move the ResetState() call here, which would be more inline with the below if?
SendState();
}
}

if (Player != null && MapEditorArea == null) {
Player = null;
PlayerBody = null;
Session = null;
WasIdle = false;
WasInteractive = false;
ResetState();
SendState();
}
return;
Expand Down Expand Up @@ -725,12 +706,9 @@ public override void Update(GameTime gameTime) {
}

if (Player == null || Player.Scene != level) {
Player = level.Tracker.GetEntity<Player>();
if (Player != null) {
PlayerBody = Player;
Session = level.Session;
WasIdle = false;
WasInteractive = false;
Player player = level.Tracker.GetEntity<Player>();
if (player != null) {
ResetState(player, level.Session);
StateUpdated |= true;
SendGraphics();
}
Expand Down Expand Up @@ -797,17 +775,14 @@ public void OnSetActualDepth(On.Monocle.Scene.orig_SetActualDepth orig, Scene sc
public void OnLoadLevel(On.Celeste.Level.orig_LoadLevel orig, Level level, Player.IntroTypes playerIntro, bool isFromLoader = false) {
orig(level, playerIntro, isFromLoader);

Session = level.Session;
WasIdle = false;
WasInteractive = false;
Player player = null;
if (Client != null)
player = level.Tracker.GetEntity<Player>();

if (Client == null)
return;
ResetState(player, level.Session);

Player = level.Tracker.GetEntity<Player>();
PlayerBody = Player;

SendState();
if (Client != null)
SendState();
}

public void OnExitLevel(Level level, LevelExit exit, LevelExit.Mode mode, Session session, HiresSnow snow) {
Expand All @@ -832,17 +807,15 @@ private Player OnLoadNewPlayer(On.Celeste.Level.orig_LoadNewPlayer orig, Vector2
private void OnPlayerAdded(On.Celeste.Player.orig_Added orig, Player self, Scene scene) {
orig(self, scene);

Session = (scene as Level)?.Session;
WasIdle = false;
WasInteractive = false;
Player = self;
PlayerBody = self;

ResetState(self, (scene as Level)?.Session);
SendState();
SendGraphics();

foreach (DataPlayerFrame frame in LastFrames.Values.ToArray())
Handle(null, frame);
// We can't directly handle the frames here, as then ghost creation logic could fail if we're currently loading a level in OnEndOfFrame
scene.OnEndOfFrame += () => {
foreach (DataPlayerFrame frame in LastFrames.Values.ToArray())
Handle(null, frame);
};
}

private PlayerDeadBody OnPlayerDie(On.Celeste.Player.orig_Die orig, Player self, Vector2 direction, bool evenIfInvincible, bool registerDeathInStats) {
Expand Down Expand Up @@ -908,6 +881,22 @@ private void ILTransitionRoutine(ILContext il) {

#endregion

public void ResetState(Player player = null, Session ses = null) {
// Clear ghosts if the scene changed
if (player?.Scene != Player?.Scene) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I can add these types of comments too, eh.

Forgot to mention this line probably needs to be changed to something like:

if (player != null && player.Scene != Player?.Scene) {

just so we don't delete all Ghosts just because Player is null, probably happens somewhere during respawning.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually my suggestion would change/break Cleanup() behavior, which sets Player to null and removes all Ghosts -- with my suggestion, the Ghost removal wouldn't happen anymore...
Maybe we should seperate the Ghost removal from ResetState?

lock (Ghosts) {
foreach (Ghost ghost in Ghosts.Values)
ghost?.RemoveSelf();
Ghosts.Clear();
}
}

Player = player;
PlayerBody = player;
Session = ses;
WasIdle = false;
WasInteractive = false;
}

#region Send

Expand Down
2 changes: 1 addition & 1 deletion CelesteNet.Client/Entities/GhostNameTag.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public override void Render() {

float scale = level.GetScreenScale();

Vector2 pos = Tracking?.Position ?? Position;
Vector2 pos = Tracking?.BottomCenter ?? Position;
pos.Y -= 16f;

pos = level.WorldToScreen(pos);
Expand Down