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

Add a waring about unsupported save files #1683

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from
Open

Add a waring about unsupported save files #1683

wants to merge 3 commits into from

Conversation

hexagonrecursion
Copy link
Contributor

@hexagonrecursion hexagonrecursion commented Jul 4, 2024 •
edited
Loading

Problem

  • Fixes Crash when loading a level after changing AI code #1623
  • Also addresses some concerns I have about a number of edge cases where public classes or public functions may cause a crash when loading a save if there were changes to the CBot API (even if the changes do not break backward compatibility).

In #1668 (comment) @Emxx52 told me "loading old save files is generally unsupported".

Solution

  1. Loading an unsuported crashsave or quicksave now behaves as if there is no save
  2. The -loadsave option also does not recognise a save as valid unless its version is supported
  3. Loading an unsupported save via the save list asks the player to confirm

Note to self: after this is merged I should implement a fix for:

@hexagonrecursion hexagonrecursion mentioned this pull request Aug 16, 2024
Open
@hexagonrecursion hexagonrecursion mentioned this pull request Aug 31, 2024
Open
@hexagonrecursion hexagonrecursion changed the title Loading old save files is unsupported Add a waring about unsupported save files Sep 2, 2024
@hexagonrecursion
Copy link
Contributor Author

@hexagonrecursion
Copy link
Contributor Author

Ready for review: no more conflicts :)

@hexagonrecursion
Copy link
Contributor Author

Who should I ask to review this?

@Emxx52 Emxx52 self-requested a review November 19, 2024 01:39
@tomaszkax86
Copy link
Contributor

I'll review this today or tomorrow.

@tomaszkax86 tomaszkax86 self-requested a review November 19, 2024 13:09
Emxx52
Emxx52 requested changes Nov 20, 2024
View reviewed changes
colobot-base/src/common/version.h.cmake Outdated
static inline constexpr int MAJOR = @CMAKE_PROJECT_VERSION_MAJOR@;
static inline constexpr int MINOR = @CMAKE_PROJECT_VERSION_MINOR@;
static inline constexpr int PATCH = @CMAKE_PROJECT_VERSION_PATCH@;
static inline constexpr int TWEAK = @CMAKE_PROJECT_VERSION_TWEAK@;
Copy link
Member

Choose a reason for hiding this comment

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

TWEAK should be removed, we will not be using it in future releases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, we had ideas for it but that's unnecessary work for little benefits. Top level project's tweak number will also be removed.

colobot-base/src/ui/screen/screen_io_read.cpp Outdated
}
else
{
m_dialog->StartQuestion("Version unsupported. Open anyway?", false, false, false, doRead);
Copy link
Member

Choose a reason for hiding this comment

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

This text should be translatable (see restext.cpp, update-pot).

tomaszkax86
tomaszkax86 requested changes Nov 20, 2024
View reviewed changes
CMakeLists.txt Outdated
Comment on lines 10 to 12
# Loading save files created by a previous version of the game is
# unsupported with one exception: the first three version components have to match
# and the fourth version component has to be greater or equal.
Copy link
Contributor

Choose a reason for hiding this comment

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

This description has no place in top level CMakeLists.txt. It should be moved near relevant implementation details, like in a comment before the function that checks save file version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing TWEAK made this comment redundant

colobot-base/src/ui/screen/screen_io_read.cpp Outdated
@@ -168,10 +171,22 @@ bool CScreenIORead::EventProcess(const Event &event)

if ( event.type == EVENT_INTERFACE_IOREAD )
{
if(IOReadScene() && m_inSimulation)
auto doRead = [&]() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: { should be in next line

colobot-base/src/ui/screen/screen_io.h Outdated
@@ -41,6 +42,7 @@ class CScreenIO : public CScreen
void IODeleteScene();
void IOWriteScene();
bool IOReadScene();
std::optional<std::filesystem::path> GetSceneName();
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: if possible, make the method const qualified

colobot-base/src/ui/screen/screen_io_read.cpp Outdated
}
else
{
m_dialog->StartQuestion("Version unsupported. Open anyway?", false, false, false, doRead);
Copy link
Contributor

@tomaszkax86 tomaszkax86 Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'd change the text to be more descriptive. Something like "This save file is from a different version of the game and might not work correctly. Do you want to open it anyway?"

@hexagonrecursion
Copy link
Contributor Author

@tomaszkax86 @Emxx52 Thanks. Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eschan145 eschan145 eschan145 approved these changes

@tomaszkax86 tomaszkax86 Awaiting requested review from tomaszkax86

@Emxx52 Emxx52 Awaiting requested review from Emxx52

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
enhancement ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Crash when loading a level after changing AI code
4 participants
@hexagonrecursion @tomaszkax86 @Emxx52 @eschan145