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

WAGE: Fix "New" and "Revert" option bugs #6436

Merged
merged 1 commit into from
Mar 11, 2025
Merged

Conversation

WinterSun23
Copy link
Contributor

No description provided.

Copy link
Member

@lephilousophe lephilousophe left a comment

Choose a reason for hiding this comment

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

I did not review the core of the changes, only the style.

Please follow our Commit Guidelines, especially on commit message formatting and linear history.

@@ -172,7 +172,26 @@ Common::Error WageEngine::run() {
return Common::kNoError;
}

//Resetting required variables
//TODO : Check if we should delete stuff like _monster,_offer before setting it back to null
Copy link
Member

Choose a reason for hiding this comment

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

Why is it a TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have just started to work on this engine and haven't fully understood the code base yet. I will fix this soon.

@bluegr bluegr changed the title Fix "New" and "Revert" option bugs WAGE: Fix "New" and "Revert" option bugs Feb 15, 2025
@sev-
Copy link
Member

sev- commented Feb 15, 2025

You need to follow our commit guidlines and code formatting guidelines. Please clean up, before we do a deeper review.

@lephilousophe lephilousophe marked this pull request as draft February 15, 2025 13:59
@WinterSun23 WinterSun23 force-pushed the master branch 3 times, most recently from 438f337 to 447a42e Compare February 16, 2025 07:03
Copy link
Member

@sev- sev- left a comment

Choose a reason for hiding this comment

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

I added some questions. It is not clear to me if the changes you added were checked against other parts of the engine.

@@ -501,10 +501,7 @@ int WageEngine::loadGame(int slotId) {
}
for (uint32 i = 0; i < orderedScenes.size(); ++i) {
Scene *scene = orderedScenes[i];
if (scene == _world->_storageScene) {
scene->_chrs.clear();
Copy link
Member

Choose a reason for hiding this comment

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

Could you please explain the rationale behind this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a bug when using the revert option: Take a object, save the game, then drop the object and then revert to the save. This caused the object to be both in the scene objects and the player inventory.

This is because the scene's characters and objects were not being cleared when we used the revert option. So I clear the objects and character of all scenes to ensure duplication is prevented.

@@ -133,6 +133,8 @@ class Gui {
void actionCut();
void disableUndo();
void disableAllMenus();
void enableAllMenus();

Copy link
Member

Choose a reason for hiding this comment

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

Why this empty line was added?

_engine->loadGameState(_engine->_defaultSaveSlot);
_engine->_gui->enableAllMenus();
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure that all menus need to be enabled? What happens when the game starts at the very beginning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think only some have to be enabled. I will correct it soon.

_scene = nullptr; // To force current scene to be redrawn
_engine->redrawScene();
g_system->updateScreen();
g_system->delayMillis(100);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this delay added here?

@@ -172,7 +172,25 @@ Common::Error WageEngine::run() {
return Common::kNoError;
}

// Resetting required variables
void WageEngine::resetAllParameters() {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe name it as resetState()? The term "parameters" is not used in this engine.

- Fix issue with "New" option after game over screen

    Fix issue, where clicking the new option after the game over screen
    causes the scene window to be not visible until scummvm is
    restarted.
    Fix by resetting game related variables in engine class
    when the new option is clicked.

- Fix issue with "Revert" option

    Fix issue, where using "Revert" when the previous save is in the
    current scene, causes the current scene to not redraw. The objects
    in the scene do not have correct ownership. They are both in the
    scene objects and in player inventory.
    Fix by forcing redraw and clearing scene objects and characters
    during loading the previous save.
@WinterSun23 WinterSun23 marked this pull request as ready for review March 10, 2025 17:57
@sev-
Copy link
Member

sev- commented Mar 11, 2025

Thank you

@sev- sev- merged commit a5a6057 into scummvm:master Mar 11, 2025
1 check passed
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.

4 participants