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

Jump to bottom

Fix #9720: Delay start of GS/AI to after loading of savegame #9745

Merged
merged 1 commit into from Dec 28, 2022
Merged

Fix #9720: Delay start of GS/AI to after loading of savegame #9745

merged 1 commit into from Dec 28, 2022

Conversation

glx22
Copy link
Contributor

@glx22 glx22 commented Dec 13, 2021 •
edited

Fixes #9720.

Motivation / Problem

See #9720 (comment)

Description

During loading, AI/GS saved data is stored in ScriptConfig.
AI/GS are started after loading is fully completed.
On AI/GS start, the saved data is put on stack right after initialisation.

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@glx22 glx22 added component: AI/Game script (squirrel) This issue is related to Squirrel (Scripting language) backport requested This PR should be backport to current release (RC / stable) needs review: Script API Review requested from GS/AI expert labels Dec 13, 2021
frosch123
frosch123 reviewed Dec 17, 2021
View reviewed changes
src/openttd.cpp Outdated Show resolved Hide resolved
src/script/script_config.cpp Outdated Show resolved Hide resolved
src/script/script_instance.hpp Outdated Show resolved Hide resolved
rubidium42
rubidium42 reviewed Dec 20, 2021
View reviewed changes
src/script/script_instance.hpp Outdated Show resolved Hide resolved
src/openttd.cpp Outdated Show resolved Hide resolved
src/script/script_instance.hpp Outdated Show resolved Hide resolved
LordAro
LordAro previously approved these changes Aug 29, 2022
View reviewed changes
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

#YOLO

@LordAro LordAro removed the backport requested This PR should be backport to current release (RC / stable) label Nov 9, 2022
@2TallTyler 2TallTyler added the work: needs rebase This Pull Request needs a rebase label Nov 24, 2022
@2TallTyler
Copy link
Member

@glx22 Needs a rebase (and someone who understands GS/AI well enough to approve/merge it 🙂 )

@2TallTyler 2TallTyler added this to the 13.0 milestone Nov 24, 2022
@2TallTyler 2TallTyler removed the work: needs rebase This Pull Request needs a rebase label Nov 25, 2022
@glx22 glx22 mentioned this pull request Dec 1, 2022
Closed
@glx22 glx22 marked this pull request as draft December 2, 2022 00:55
@glx22
Copy link
Contributor Author

glx22 commented Dec 2, 2022

Back to draft because #10206 (comment)

@glx22 glx22 marked this pull request as ready for review December 3, 2022 00:31
@glx22
Copy link
Contributor Author

glx22 commented Dec 3, 2022 •
edited

Scripts are already not started by first commit but the saved data is still in memory and without setting c->is_ai to false OpenTTD assumes a script is started and crashes during game loop.
Maybe I should just merge both commits.

@glx22 glx22 marked this pull request as draft December 14, 2022 22:09
@glx22 glx22 mentioned this pull request Dec 15, 2022
Merged
@2TallTyler 2TallTyler removed this from the 13.0 milestone Dec 15, 2022
@glx22 glx22 marked this pull request as ready for review December 15, 2022 21:42
@glx22
Copy link
Contributor Author

glx22 commented Dec 15, 2022

#10241 already fixed the crash reported in #9720, but here I'm addressing #9720 (comment)

@2TallTyler 2TallTyler added this to the 13.0 milestone Dec 16, 2022
rubidium42
rubidium42 reviewed Dec 27, 2022
View reviewed changes
src/script/script_instance.cpp Outdated
return false;
}

default: NOT_REACHED();
Copy link
Contributor

Choose a reason for hiding this comment

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

_script_sl_byte is coming from the savegame, isn't it? So might throwing some error/exception that the data is corrupt be a better option than crashing the game?

src/script/script_instance.hpp
*/
void Load(int version);
static ScriptData *Load(int version);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not quite fond of the two Load functions with the same name and wildly different behaviours. What about something like LoadFromSavegame and LoadIntoInstance (for the next function)?

@glx22 glx22 merged commit fe30f66 into OpenTTD:master Dec 28, 2022
@glx22 glx22 deleted the fix_9720 branch December 28, 2022 04:02
@glx22 glx22 mentioned this pull request Jan 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frosch123 frosch123 frosch123 left review comments

@rubidium42 rubidium42 rubidium42 approved these changes

@LordAro LordAro LordAro left review comments

Assignees
No one assigned
Labels
component: AI/Game script (squirrel) This issue is related to Squirrel (Scripting language) needs review: Script API Review requested from GS/AI expert
Projects
None yet
Milestone
13.0
Development

Successfully merging this pull request may close these issues.

[Crash]: Can't start game with CityGrowthLimiter GS
5 participants
@glx22 @2TallTyler @LordAro @rubidium42 @frosch123