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

Save system problem during campaign play #842

Closed
DelphiDie opened this issue Jun 13, 2018 · 24 comments
Closed

Save system problem during campaign play #842

DelphiDie opened this issue Jun 13, 2018 · 24 comments
Milestone

Comments

@DelphiDie
Copy link

Description of Problem or Question

From the feedback of multiple users and some tests of my own, it seems very likely that there is a problem with the way Valkyrie's saving/loading system works in regards to D2 campaign play.

While the campaign works absolutely fine in Valkyrie during play, a problem occurs after loading a save file. Then the game just stops at the point, where a new quest.ini is supposed to be called.

My guesses would be, that either the save system does not yet take into account that quests can now start other quests, and is thus missing some required information, or that Valkyrie might have a problem getting the correct file path to call the next quest after loading a game.

To reproduce the error simply download "The Shadow Rune" from within Valkyrie, start a game, save the game, load this save and continue the quest. At the point where the next quest is then supposed to start nothing happens.

Due to the gravity of this problem playing a campaign with Valkyrie is currently practically impossible, and it would be great to get this problem solved as soon as possible.

Valkyrie Version

2.0.1

@BenRQ
Copy link
Collaborator

BenRQ commented Aug 30, 2018

Hi @DelphiDie @MrTantum

I do not reproduce the issue.
Loading takes some time but it loads for me.

I did this:

  • start "Shadow rune act 1" and save immediately
  • load the quest
  • 'defeat' all monsters, said 0 zombies escaped
  • pressed continue
  • wait 10 seconds : it loads the next quest

Did I miss something ?

@BenRQ
Copy link
Collaborator

BenRQ commented Aug 30, 2018

Can you please share your savegame so I can try with your file ?

@DelphiDie
Copy link
Author

DelphiDie commented Aug 31, 2018

Hello @BenRQ ,

thanks for looking at this.

It sounds like you tried the quest "The Shadow Rune - Act 1 The Cardinals Plight".

This quest was not made by us, and I'm unsure if the problem can be reproduced here, since this seems to be a single quest.

Could you please try it again with the Quest "The Shadow Rune"?

If needed I can provide you a savegame, but it's pretty easy to reproduce.

@BenRQ
Copy link
Collaborator

BenRQ commented Aug 31, 2018

After a quick investigation, it seems that the save system only copies the current quest.
As the current loaded quest is running in a temporary directory, you can't access any other quest from there.

I have the explanation but not the fix, and I can't say how complex it will be to fix it.
There is probably a reason for not using the quest content in the original directory but use everything in the save directory, but I don't know it.

If we can reference the original directory, then it would also fix #809, and you won't have to use the ../
Your event would just look like this :
event1=CP/quest.ini
instead of
event1=../CP/quest.ini

@NPBruce if you have a bit of time to give us your opinion on this?
I don't want to start changing the save system and lose my time working in the wrong direction :)

@redwolf2
Copy link
Collaborator

redwolf2 commented Aug 31, 2018

The reason for the temp folder lies within the valkyrie packages. They get extracted there, when you load a scenario. The temp directory is deleted every time when the app starts.

The scenarios were in folders, before valkyrie packages were introduced. That change may have broken the campaign functionality.

Maybe we can modify the valkyrie packages to hold multiple scenarios? Auto download the right scenario? Auto extract the right scenario? Have a message, when a scenario is missing and the user must download it manually?

Maybe we could auto generate a GUID and put it in the quest.ini. That way we can check if the scenario is installed locally and then have a message, if it's not.

To stay compatible, we could just interpret the last part of the path (like you suggested) and crawl the zip folder structure and the directories (user created senarios are still stored in a folder structure). I have already done the zip crawling in that functionality, which extracts the valkyrie version info on android. See here:

internal string ExtractObbVersion()

@BenRQ
Copy link
Collaborator

BenRQ commented Aug 31, 2018

OK I see : the app extracts all packages content everytime you open the list of quests by pressing "start quest". That's why it takes a bit of time to load.
When loading a game, it loads only the savegame packages which is incomplete (you can see it if you manually unzip the archive).
And this savegame is NOT extracted in the same directory by the way : AppData\Local\Temp\Valkyrie\Load\quest

Savegame packages should definitely hold the full scenario and not only the current quest.
Now it seems a bit extreme to save the full package everytime doesn't it ?

And I like the idea of the autodownload. I've started thinking how we could merge the list of quest and the download page already. This is another topic though, for a future release :)

@redwolf2
Copy link
Collaborator

redwolf2 commented Aug 31, 2018

A reason not to bundle all scenarios in the valkyrie package could be size and complexity. Another reason might be, that the full scenario is still not complete and some quests will be completed by the scenario editor, later.

@BenRQ
Copy link
Collaborator

BenRQ commented Sep 1, 2018

I see three 'small' changes in the feature design that would fix this issue, issue #809 and @scrubbless performance issue on his last scenario :

  1. the quest package is not fully extracted when displaying the quest list, only required files ( thanks for the way to do it @redwolf2 )
  2. save feature would only write the save.ini without the quest package (risk of incoherence with the scenario when the quest is being updated)
  3. the full package is only extracted when starting a quest, or loading a savegame

Risk pointed out in the sec2 is not a blocker from my point of view as update is not automatic, and the issue is kind of obvious for me, but please tell me what you think.

Improvement on performance during game, and during quest selection would be quite high.
There would be small performance impact when loading / starting a scenario.

@redwolf2
Copy link
Collaborator

redwolf2 commented Sep 1, 2018

I like the changes.

Android would also benefit from deleting the temp scenario dir after the scenario is no longer in use. Storage space can be sparse, especially internal storage. This is not a huge priority, but could prevent problems in edge cases.

Risk pointed out in the sec2 is not a blocker from my point of view as update is not automatic, and the issue is kind of obvious for me, but please tell me what you think.

Before an update you could have a warning, that there are save files that may not work or only allow the update if the save is killed.

#809 might be due to windows path restictions of 255 characters max. https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file. It would be better not to rely on the file path at all since that path will be different on every platform. This may lead to save files might not be compatible among different platforms. A GUID per package (as mentioned before) would be nice and if there is none, we could interpret the last part of the path to be compatible with quest formats below. Issue 809 might be fixed by not beeing relieant on those ..\ path backnavigation, but resolving to the absolute path or implement a custom resolver if necessary.

@MrTantum
Copy link

MrTantum commented Sep 2, 2018

Thanks for looking into this. Your comments show that this is not an easy bugfix. Would be fantastic if you are able to solve this. 👍

BenRQ added a commit to BenRQ/valkyrie that referenced this issue Sep 2, 2018
- Savegames do not contain the full quest anymore (performance improvement)
- When listing quests, just extract necessary files from packages (performance improvement)
- When listing quests, search for content in the editor of game type directory and in Download directory
- Only when starting quest, extract the full archive
- NPBruce#842: loading from a savegame will extract the full archive
- NPBruce#809: external quest path are now based on root quest (you should not use ../ anymore)
- Do not extract all quests packages available when loading a quest from a quest : a quest can be started from another quest only if they all are in the same quest archive

Limitations :
- Updating a quest that is saved may break your savegame (todo: add warning)
- Deleting a quest that is saved *will* break your savegame (todo: add warning)
@BenRQ
Copy link
Collaborator

BenRQ commented Sep 2, 2018

I spent some time on this, and I think I now have something working.
It will be available for test in the beta Valkyrie Brynhildr_2.1.0a.

I did not implement any warning when deleting/updating quest.
It's a small dev, but I'd rather concentrate on releasing the beta for now.

@DelphiDie
Copy link
Author

Thanks for your efforts BenRQ.

Once we have an official release I will update the quest structure in our campaign.

Regards
Delphi

BenRQ added a commit to BenRQ/valkyrie that referenced this issue Sep 3, 2018
- Savegames do not contain the full quest anymore (performance improvement)
- When listing quests, just extract necessary files from packages (performance improvement)
- When listing quests, search for content in the editor of game type directory and in Download directory
- Only when starting quest, extract the full archive
- NPBruce#842: loading from a savegame will extract the full archive
- NPBruce#809: external quest path are now based on root quest (you should not use ../ anymore)
- Do not extract all quests packages available when loading a quest from a quest : a quest can be started from another quest only if they all are in the same quest archive

Limitations :
- Updating a quest that is saved may break your savegame (todo: add warning)
- Deleting a quest that is saved *will* break your savegame (todo: add warning)
@BenRQ
Copy link
Collaborator

BenRQ commented Sep 3, 2018

Beta test version is now available with implementation of this issue:
https://github.com/BenRQ/valkyrie/releases/tag/release%2F2.1%2FBrynhildr_2.1.0b

I suggest you point out the required Valkyrie version in the description of the scenario, when version is officially released.

Also please close this issue if you are happy with the fix.

@DelphiDie
Copy link
Author

DelphiDie commented Sep 4, 2018

Hello @BenRQ,

I reworked the quests to point to absolute paths, and they work flawlessly with the beta release. Also saving and loading does no longer break the campaign and works as expected.

This is such good news. I already prepared a new campaign release and can push it as soon as there is a new official Valkyrie release. Thanks so much for fixing this problem!

Regards
Delphi

@redwolf2
Copy link
Collaborator

redwolf2 commented Sep 5, 2018

Reopen, because it is fixed in Brynhildr, but not in Valkyrie. I created the label "Brynhildr" to track, what is covered by it.

@redwolf2 redwolf2 reopened this Sep 5, 2018
@DelphiDie
Copy link
Author

Thanks @redwolf2 & @BenRQ. Would you happen to know is there is a new official release in development?

@BenRQ: Could I distribute the Brynhildr release as a workaround via direct download (for ease of use), or would you prefer that I link to your release section?

Regards,
Delphi

@redwolf2
Copy link
Collaborator

redwolf2 commented Sep 6, 2018

@NPBruce is busy at least this year. After that he might pick up where he left. So no official releases until he is back. Development continues in @BenRQ branch, which Bruce might just merge into his (I know I would).

@BenRQ
Copy link
Collaborator

BenRQ commented Sep 6, 2018

Hi @DelphiDie, I think it is better to share the link to the release section, as you don't know what is the system the player will be using (windows, android, mac ...)

@DelphiDie
Copy link
Author

Thanks to both of you. I guess I will post a Link to the Brynhildr Release then. The OS is indeed something I hadn't thought about and a very valid point.

BenRQ added a commit to BenRQ/valkyrie that referenced this issue Sep 13, 2018
- Improve pre-loading of savegame list
- secure path construction for quest within quest
- remove useless information in savefile and quest structure
- Do not try to extract when loading a scenario not packaged
- Remove old code
- fix missing new line in savegame for duration
- Fix issue where quest list screen would not appear when no quests have ever been downloaded
BenRQ added a commit to BenRQ/valkyrie that referenced this issue Sep 25, 2018
…ework

- Quest is now saved in save files only when required
- Quest is now saved in a separate thread to avoid long end of turn (new class ZipManager)
- Improve pre-loading of savegame list, create a preload directory in Temp directory for this
- secure path construction for quest within quest
- remove useless information in savefile and quest structure
- Do not try to extract when loading a scenario not packaged
- Remove old code
- fix missing new line in savegame for duration
- Fix issue where quest list screen would not appear when no quests have ever been downloaded
- Fix loading twice quest data
BenRQ added a commit to BenRQ/valkyrie that referenced this issue Sep 28, 2018
- fix issue with android extraction in subfolder
- clean code, remove logs, add interesting logs
BenRQ added a commit to BenRQ/valkyrie that referenced this issue Sep 29, 2018
* NPBruce#111 
- Fix issue with descent end screen not showing up: set a background image for end screen in Descent
- use originalPath in the stats: the quest name can change for quests with multiple scenarios so it's better to use the originalPath
- remove some logs
- scenario should not use default directory name
Only scenario with original names will get the rating screen at the end of the game, and can push stats. Current scenario using EditorScenario should be warned to change their scenario name.
-  only support packages

* NPBruce#859 [MoM] Do not display button bar in the editor

* NPBruce#893 Do not display autosave slot even when no saves exists yet

* NPBruce#891 Fix Tokens being covered by object after save/load or undo

The type Dictionary<T1, T2> used to store the list of board items does not retain order.
The class OrderedDictionary does not exist unfortunately.
So add a 'sister' List of object name, and use this to write the savegame.

* Fix  NPBruce#841:
Issue : I found if you are in the horror phase and open an item to inspect it. When you finish that event it ends the phase
  Fix :
  A new test has been added : if in horror phase and monster > 0, then do not end the round.
  In that case you will need to press next phase button to end the round (as you should).

  Please note I also have enabled the use of all elements during horror phase except for tokens, otherwise UI element can block the screen : they will be displayed, but you won't be able to remove them.
  I checked this, and this is the behavior of the official game app : you can use items during that phase.

* Fix  NPBruce#892 Monster breaks investigator phase
Double check the validity of monster events before sending the event. By checking this, we can now directly go to horror phase and not get stuck in monster phase.
Fix tested with normal monsters, custom monsters, custominvalid monsters and combination of the three

*  NPBruce#218 Log is inaccessible during Mythos Phase
Access to logs is now authorized

* FIX / EVO NPBruce#842 NPBruce#809 New load / save system bugfix and rework
- Make sure we don't get stuck in monster phase (this should never happen)
- Quest is now saved in save files only when required
- Quest is now saved in a separate thread to avoid long end of turn (new class ZipManager)
- Improve pre-loading of savegame list, create a preload directory in Temp directory for this
- secure path construction for quest within quest
- remove useless information in savefile and quest structure
- Do not try to extract when loading a scenario not packaged
- Remove old code
- fix missing new line in savegame for duration
- Fix issue where quest list screen would not appear when no quests have ever been downloaded
- Fix loading twice quest data
- fix issue with android extraction in subfolder
- clean code, remove logs, add interesting logs

* Cleanup code

* Small fix on French translation

* Cleanup on Path creation
@redwolf2 redwolf2 added this to the 2.1 milestone Oct 5, 2018
BenRQ added a commit that referenced this issue Oct 7, 2018
- Savegames do not contain the full quest anymore (performance improvement)
- When listing quests, just extract necessary files from packages (performance improvement)
- When listing quests, search for content in the editor of game type directory and in Download directory
- Only when starting quest, extract the full archive
- #842: loading from a savegame will extract the full archive
- #809: external quest path are now based on root quest (you should not use ../ anymore)
- Do not extract all quests packages available when loading a quest from a quest : a quest can be started from another quest only if they all are in the same quest archive

Limitations :
- Updating a quest that is saved may break your savegame (todo: add warning)
- Deleting a quest that is saved *will* break your savegame (todo: add warning)
BenRQ added a commit that referenced this issue Oct 7, 2018
* #111 
- Fix issue with descent end screen not showing up: set a background image for end screen in Descent
- use originalPath in the stats: the quest name can change for quests with multiple scenarios so it's better to use the originalPath
- remove some logs
- scenario should not use default directory name
Only scenario with original names will get the rating screen at the end of the game, and can push stats. Current scenario using EditorScenario should be warned to change their scenario name.
-  only support packages

* #859 [MoM] Do not display button bar in the editor

* #893 Do not display autosave slot even when no saves exists yet

* #891 Fix Tokens being covered by object after save/load or undo

The type Dictionary<T1, T2> used to store the list of board items does not retain order.
The class OrderedDictionary does not exist unfortunately.
So add a 'sister' List of object name, and use this to write the savegame.

* Fix  #841:
Issue : I found if you are in the horror phase and open an item to inspect it. When you finish that event it ends the phase
  Fix :
  A new test has been added : if in horror phase and monster > 0, then do not end the round.
  In that case you will need to press next phase button to end the round (as you should).

  Please note I also have enabled the use of all elements during horror phase except for tokens, otherwise UI element can block the screen : they will be displayed, but you won't be able to remove them.
  I checked this, and this is the behavior of the official game app : you can use items during that phase.

* Fix  #892 Monster breaks investigator phase
Double check the validity of monster events before sending the event. By checking this, we can now directly go to horror phase and not get stuck in monster phase.
Fix tested with normal monsters, custom monsters, custominvalid monsters and combination of the three

*  #218 Log is inaccessible during Mythos Phase
Access to logs is now authorized

* FIX / EVO #842 #809 New load / save system bugfix and rework
- Make sure we don't get stuck in monster phase (this should never happen)
- Quest is now saved in save files only when required
- Quest is now saved in a separate thread to avoid long end of turn (new class ZipManager)
- Improve pre-loading of savegame list, create a preload directory in Temp directory for this
- secure path construction for quest within quest
- remove useless information in savefile and quest structure
- Do not try to extract when loading a scenario not packaged
- Remove old code
- fix missing new line in savegame for duration
- Fix issue where quest list screen would not appear when no quests have ever been downloaded
- Fix loading twice quest data
- fix issue with android extraction in subfolder
- clean code, remove logs, add interesting logs

* Cleanup code

* Small fix on French translation

* Cleanup on Path creation
@redwolf2 redwolf2 closed this as completed Oct 8, 2018
BenRQ added a commit that referenced this issue Oct 12, 2018
- Savegames do not contain the full quest anymore (performance improvement)
- When listing quests, just extract necessary files from packages (performance improvement)
- When listing quests, search for content in the editor of game type directory and in Download directory
- Only when starting quest, extract the full archive
- #842: loading from a savegame will extract the full archive
- #809: external quest path are now based on root quest (you should not use ../ anymore)
- Do not extract all quests packages available when loading a quest from a quest : a quest can be started from another quest only if they all are in the same quest archive

Limitations :
- Updating a quest that is saved may break your savegame (todo: add warning)
- Deleting a quest that is saved *will* break your savegame (todo: add warning)
BenRQ added a commit that referenced this issue Oct 12, 2018
* #111 
- Fix issue with descent end screen not showing up: set a background image for end screen in Descent
- use originalPath in the stats: the quest name can change for quests with multiple scenarios so it's better to use the originalPath
- remove some logs
- scenario should not use default directory name
Only scenario with original names will get the rating screen at the end of the game, and can push stats. Current scenario using EditorScenario should be warned to change their scenario name.
-  only support packages

* #859 [MoM] Do not display button bar in the editor

* #893 Do not display autosave slot even when no saves exists yet

* #891 Fix Tokens being covered by object after save/load or undo

The type Dictionary<T1, T2> used to store the list of board items does not retain order.
The class OrderedDictionary does not exist unfortunately.
So add a 'sister' List of object name, and use this to write the savegame.

* Fix  #841:
Issue : I found if you are in the horror phase and open an item to inspect it. When you finish that event it ends the phase
  Fix :
  A new test has been added : if in horror phase and monster > 0, then do not end the round.
  In that case you will need to press next phase button to end the round (as you should).

  Please note I also have enabled the use of all elements during horror phase except for tokens, otherwise UI element can block the screen : they will be displayed, but you won't be able to remove them.
  I checked this, and this is the behavior of the official game app : you can use items during that phase.

* Fix  #892 Monster breaks investigator phase
Double check the validity of monster events before sending the event. By checking this, we can now directly go to horror phase and not get stuck in monster phase.
Fix tested with normal monsters, custom monsters, custominvalid monsters and combination of the three

*  #218 Log is inaccessible during Mythos Phase
Access to logs is now authorized

* FIX / EVO #842 #809 New load / save system bugfix and rework
- Make sure we don't get stuck in monster phase (this should never happen)
- Quest is now saved in save files only when required
- Quest is now saved in a separate thread to avoid long end of turn (new class ZipManager)
- Improve pre-loading of savegame list, create a preload directory in Temp directory for this
- secure path construction for quest within quest
- remove useless information in savefile and quest structure
- Do not try to extract when loading a scenario not packaged
- Remove old code
- fix missing new line in savegame for duration
- Fix issue where quest list screen would not appear when no quests have ever been downloaded
- Fix loading twice quest data
- fix issue with android extraction in subfolder
- clean code, remove logs, add interesting logs

* Cleanup code

* Small fix on French translation

* Cleanup on Path creation
BenRQ added a commit that referenced this issue Oct 12, 2018
- do not mix autosave, and manual save
- do not reload Qst when going through saved quest
@DelphiDie
Copy link
Author

DelphiDie commented Jun 17, 2019

@BenRQ

Sorry to bother you again with this, but the issue seems to have reappeared in the latest Valkyrie release.

After saving the game, quitting Valkyrie, restarting it and the reloading the save, that game can not be continued as intended, because the next quest.ini call after loading does not happen.

The following steps can be done to reproduce the issue:

  1. Start Valkyrie Descent Module
  2. Select New Game
  3. Download and start "The Shadow Rune" (with any hero combination)
  4. Choose "Forfeit Quest"
  5. Choose "Campaign Phase"
  6. Select the City
  7. Leave the City
  8. Save the game
  9. Quit to Main Menu
  10. Quit Valkyrie
  11. Restart Valkyrie Descent Module
  12. Click on Continue
  13. Choose the savegame
  14. Click on any of the available quests
  15. Resolve the travel phase
  16. After this point the game should call a new quest.ini and then hangs

Note: If you just continue the game after step 8 or load the savegame from the main menu after step 9, everything seems to work fine. Closing and restarting Valkyrie however leads to the problem.

This is of course a very serious issue, because the save system is pretty much useless in this case.

Could you please take a look at the problem?

@redwolf2 Could you perhaps reopen this issue until further notice?

@vogella
Copy link

vogella commented Jul 5, 2019

@redwolf2 @BenRQ I also get stuck in a scenario if I save, leave and continue to play. See #1174

@redwolf2 redwolf2 reopened this Jul 8, 2019
@NPBruce NPBruce modified the milestones: 2.1, 2.4 Jul 30, 2019
@NPBruce
Copy link
Owner

NPBruce commented Aug 8, 2019

Fixed in 2.4

@NPBruce NPBruce closed this as completed Aug 8, 2019
@vogella
Copy link

vogella commented Aug 8, 2019

Thanks a bunch @NPBruce. Will test over the weekend. Really looking forward to be able to play the custom campagn

@vogella
Copy link

vogella commented Aug 26, 2019

Thanks for the fix @NPBruce saving and restarting works now perfectly.

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

No branches or pull requests

6 participants