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.

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

Fixed Badeline last node crash #657

Merged
merged 6 commits into from
Aug 5, 2023

Conversation

nhruo123
Copy link
Contributor

@nhruo123 nhruo123 commented Aug 5, 2023

This PR fixes an issue where if the player this the last node of FinalBoss (Badeline) the game crashes.

Additinal details:
https://discord.com/channels/403698615446536203/1137280308887175178

@DemoJameson
Copy link
Member

don't modify the OnPlayer(), since SJ il hook this method
maybe move your code into PushPlayer()

@nhruo123
Copy link
Contributor Author

nhruo123 commented Aug 5, 2023

That could work, we could also patch it at the IL level and call some kind of static helper function to disply the card, not sure whats better.

Celeste.Mod.mm/Patches/FinalBoss.cs Outdated Show resolved Hide resolved
@nhruo123 nhruo123 requested a review from DemoJameson August 5, 2023 14:16
@DemoJameson DemoJameson merged commit 09fca26 into EverestAPI:dev Aug 5, 2023
@nhruo123 nhruo123 deleted the fix-badeline-crash branch August 5, 2023 15:30
@DemoJameson
Copy link
Member

// we want to call LevelEnter.Go explicitly instant of letting CheckForErrors call it to prevent double triggers of postcard
LevelEnter.Go((Scene as Level).Session, false);

why the postcard will be triggered twice, since CheckForErrors only called once every frame

@nhruo123
Copy link
Contributor Author

nhruo123 commented Aug 5, 2023

// we want to call LevelEnter.Go explicitly instant of letting CheckForErrors call it to prevent double triggers of postcard
LevelEnter.Go((Scene as Level).Session, false);

why the postcard will be triggered twice, since CheckForErrors only called once every frame

I used to set patch_LevelEnter.ErrorMessage at OnPlayer it was getting called after CheckForErrors consumed the error message somehow (I'll add a video that shows the issue), I didn't take the time to debug it and I just worked around it.

But now when its on PushPlayer that is called from Coroutine MoveSequence it does work, so it can be removed.

Is this behaviour with OnPlayer and patch_LevelEnter.ErrorMessage expected or should I debug it tomorrow?

(08/05/2023 21:51:11) [Everest] [Info] [LevelLoader] Loading bossfight
(08/05/2023 21:51:19) [Everest] [Warn] [FinalBoss] FinalBoss entity was hit on its last node, please add an additional node outside of the current room to ensure the player never hits it.
(08/05/2023 21:51:19) [Everest] [Warn] [FinalBoss] FinalBoss entity was hit on its last node, please add an additional node outside of the current room to ensure the player never hits it.
(08/05/2023 21:51:33) [Everest] [Info] [LevelLoader] Loading bossfight
output.mp4

@nhruo123
Copy link
Contributor Author

nhruo123 commented Aug 5, 2023

Okay I figured out what's going on. it seems that even after CheckForErrors finds an error it keeps updating updating the level, so the state of patch_LevelEnter.ErrorMessage is as follows:

  1. first update: patch_LevelEnter.ErrorMessage get set by OnPlayer
  2. second update: patch_LevelEnter.ErrorMessage gets cleared but CheckForErrors
  3. second update: patch_LevelEnter.ErrorMessage gets set again by a call to OnPlayer

then we leave the level with patch_LevelEnter.ErrorMessage already set.
I am not 100% sure about it but I feel like if we just stop updating the scene after we find an error at CheckForErrors it will work as expected, If it will I'll open a new PR for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants