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

Fix SupportPowerBotModule references to invalid support powers #17060

Merged
merged 2 commits into from Oct 1, 2019

Conversation

@pchote
Copy link
Member

commented Sep 7, 2019

Fixes #16995.

The issue happens because SupportPowerBotModule doesn't clean up references to invalid power instances (associated with structures that have been killed). These pile up over time, are added to the save game, and then crash when trying to restore them.

The first commit adds a sanity check during loading to ignore any invalid powers. The second commit removes them properly, which should allow the GC to reclaim those objects and anything they reference.

@pchote pchote added this to the Next Release milestone Sep 7, 2019
@pchote pchote changed the title Fix bot support Fix SupportPowerBotModule references to invalid support powers Sep 7, 2019
@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

Could it be that you meant to fix #16995 (this one is related to save games)?

@pchote

This comment has been minimized.

Copy link
Member Author

commented Sep 7, 2019

Yes, good catch.

Copy link
Member

left a comment

Code changes look good to me.

Copy link
Contributor

left a comment

I did the following test in TD: Spectate an 4v4 AI game and save after the first nukes were fired. Then leave the game and load the saved game. The loading screen finished and the score screen was shown. But I couldn't click "Resume" - all input was ignored. After a while the game crashed with a desync exception. The same happened when doing the test in RA. I only attached the exception log from the TD game as it is aside from the frame number identical to the one for RA. Also attached a Screenshot of the score screen shortly before it crashed and the save files.
files.zip

I'm not sure if this is an issue related to the changes in this PR or a different issue.

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

Tbh, the game crashing with a desync sound like the current desync issue not related to the crash this PR fixes.

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2019

I can't reliably test this PR because my attempt also resulted in a desync, one during replay and one at the end of loading the savegame of a full 4-AI skirmish.

Code looks good at first glance, but not sure if we should merge this before bleed's desync issue is fixed so we can re-test and be sure this PR doesn't introduce a desync of its own (the logs didn't give any useful hints at what exactly caused the desync).

@teinarss teinarss merged commit 865d8d7 into OpenRA:bleed Oct 1, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.