Skip to content

Fix Portraits.xml reloading in unintended scenarios #919

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

Conversation

SnipUndercover
Copy link
Contributor

@SnipUndercover SnipUndercover commented Jun 10, 2025

Introduced by #890, portraits are reloaded on any scene change. This causes the portraits to reload in unintended scenarios, like reloading the map via F5, opening the Debug Map, or hitting any of the hardcoded F1-F3 binds.

This PR fixes this unintended behavior. Tested locally and it works.

@SnipUndercover SnipUndercover changed the title No arh or debug map portrait reload Fix Portraits.xml reloading in unintended scenarios Jun 10, 2025
@maddie480-bot maddie480-bot added the review needed This PR needs 2 approvals to be merged (bot-managed) label Jun 10, 2025
@Wartori54
Copy link
Member

Since this is a detail that we all missed in the first pr, and requires a separate fix because it was not found until later, I believe it's worthy to be referenced in some way in the added code, specifically why those scenes should be excluded and for what reason.

@SnipUndercover
Copy link
Contributor Author

My first attempt with just MapEditor (Debug Map) and AssetReloadHelper (F5) apparently didn't work. I checked and the scene seems to also be changed to LevelLoader (hardcoded F1-F3 binds, the softlock prevention, etc) or LevelExit (restart chapter / dying with a golden), and adding those two worked for both me and the person that experienced the issue.
I'll add the comments later today.

@SnipUndercover SnipUndercover requested a review from Wartori54 June 12, 2025 21:37
@maddie480-bot maddie480-bot added changes requested This PR cannot be merged because changes were requested (bot-managed) and removed review needed This PR needs 2 approvals to be merged (bot-managed) labels Jun 14, 2025
@SnipUndercover SnipUndercover requested a review from Wartori54 June 20, 2025 22:23
@maddie480-bot maddie480-bot added review needed This PR needs 2 approvals to be merged (bot-managed) and removed changes requested This PR cannot be merged because changes were requested (bot-managed) labels Jun 23, 2025
Copy link
Contributor

@DashingCat DashingCat left a comment

Choose a reason for hiding this comment

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

I tested the changes on my end, I confirm that before applying the changes, the unexpected reloading of portraits appears in F1-F3 and F5 scenarios, but I couldn't replicate it with the Debug Map.

This PR fixes the unexpected reloading in F1-F3 and F5 scenarios.

@SnipUndercover
Copy link
Contributor Author

Reverted the LevelExit changes.
I kinda wanted to leave them in considering that this seems useful to have public, even though publicizer exists.

@maddie480-bot
Copy link
Member

The pull request was approved and entered the 3-day last-call window.
If no further reviews happen, it will end on Jul 16, 2025, 12:08 AM UTC, after which the pull request will be able to be merged.

@maddie480-bot maddie480-bot added last call window This PR was approved, and is in the 5-day last-call window before getting merged (bot-managed) and removed review needed This PR needs 2 approvals to be merged (bot-managed) labels Jul 13, 2025
@maddie480-bot
Copy link
Member

The last-call window for this pull request ended. It can now be merged if no blockers were brought up.

@maddie480-bot maddie480-bot added ready to merge This PR was approved and the last-call window is over (bot-managed) and removed last call window This PR was approved, and is in the 5-day last-call window before getting merged (bot-managed) labels Jul 16, 2025
@SnipUndercover
Copy link
Contributor Author

let's go

@SnipUndercover SnipUndercover merged commit 6b8b9fc into EverestAPI:dev Jul 16, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge This PR was approved and the last-call window is over (bot-managed)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants