adventuregamestudio / ags

AGS editor and engine source code
Other
672 stars 159 forks source link

Game project becomes inconsistent if not saved after deleting translation #2414

Open Crystal-Shard opened 3 weeks ago

Crystal-Shard commented 3 weeks ago

Describe the bug Sometimes the editor will save the Game.agf file when the user doesn't ask to. This behavior is inconsistent and can break the game.

AGS Version 3.6.0

Game n/a

To Reproduce Steps to reproduce the behavior:

  1. Open any game
  2. Create a new translation
  3. save now (I had missed the fact that filling the TRS file automatically saves the game)
  4. Delete the translation
  5. Quit the editor without saving
  6. It turns out that the game was quietly saved after step 2 anyway; following these steps will result in an unreadable game.agf file

Expected behavior Either (1) do not save unless the user presses save, or (2) be consistent when to save (e.g. BOTH when creating a translation AND when deleting the translation).

Desktop (please complete the following information):

ericoporto commented 3 weeks ago

following these steps will result in an unreadable game.agf file

I think this was fixed in AGS 3.6.1 already.

https://github.com/adventuregamestudio/ags/blob/ced94199c8e575673ae8f9a6713c71038cd6d96d/Changes.txt#L147

See #2208

ivan-mogilko commented 3 weeks ago

I tried to replicate this in 3.6.0. (Running 3.6.0.58)

Editor does not save when adding translations. On contrary, it warns you on exit if translation was created, but game not saved:

Save changes?

Files have been added, removed or renamed. If you don't save the game now, you may not be able to successfully open this game next time. Do you want to save your changes?

Same happens if you create and then instantly delete a translation, then quit.

Editor saves when you do "Update Translation", and warns about that too:

Updating the translation can take some time depending on the size of your game, and will save your game beforehand. Do you want to continue?

@Crystal-Shard , please double check your steps to reproduce this problem.

Crystal-Shard commented 3 weeks ago

Ok, I've double-checked and the editor does give warnings. But I do find this approach problematic;

First, when creating a translation, I get a query whether I'd like to fill the new translation with existing text. Note that this is a false choice for the user: if I don't do this, then the translation is completely pointless.

Second, when exiting the editor, I get a warning that if I don't save now, this might break the game. Note that again this is a false choice: if I don't do this, then I end up with a broken and unusable .AGF file.

I'm not a fan of choices like these. If there's and A-or-B choice and choosing A will cause the game (or translation) to fail, then this should not be asked of the user, and B should be automatic.

ericoporto commented 3 weeks ago

Can you create a small example of such broken agf file? I can't figure out any case that causes this with the provided steps.

ivan-mogilko commented 3 weeks ago

Second, when exiting the editor, I get a warning that if I don't save now, this might break the game. Note that again this is a false choice: if I don't do this, then I end up with a broken and unusable .AGF file.

This is not accurate, if you don't save, then agf won't be broken, there will be a unused translation file remaining in a folder, but agf won't contain anything invalid (since it was not saved).

Same thing happens if you create a room, and don't save: a room file remains, but the agf is not invalid, it simply does not mention this room.

Crystal-Shard commented 3 weeks ago

This is not accurate, if you don't save, then agf won't be broken, there will be a unused translation file remaining in a folder, but agf won't contain anything invalid.

It's the reverse: if you delete the TRS from the editor, then it gets removed from disk. If you then do not save the game, then the .AGF file will contain a reference to a file that does not exist, and will refuse to load (although as mentioned above, in 3.6.1 this is fixed).

(edit) Step 3 in my initial report is inaccurate, because I had missed the fact that filling the TRS file automatically saves the game.

ivan-mogilko commented 3 weeks ago

Okay, I see now, so this is when you

Probably same may happen with other items, like rooms and fonts.

Crystal-Shard commented 3 weeks ago

Okay, I see now, so this is when you

  • created Translation
  • saved the game
  • deleted Translation
  • quit without saving.

Yes. So my issue here is with the warning "If you don't save the game now, you may not be able to successfully open this game next time."

Instead of this warning, simply save the game. The game is also saved automatically when you compile, when you fill a TRS file, and probably in a few other situations. It's not really helpful to ask the user "If you don't do X, your game file will be unusable. Do you want to X?"

ivan-mogilko commented 3 weeks ago

Alright, so there are two things

  1. Make sure that Editor does not ask wrong questions.
  2. Make sure that the project may be loaded if some optional parts are missing. This is a standalone task, because these files may get deleted by hand.

Loading a project with missing translations was fixed in 3.6.1. I recall that I also fixed loading with missing scripts, either in 3.6.0 or 3.6.1. Missing spritefile may be recreated (if sources exist), but in any case that does not prevent opening a project. Missing audio files in AudioCache may be automatically recreated as well, and also don't prevent opening a project. What is left to check:

Regarding the version, since 3.6.1 was released, I'm now not patching previous versions like 3.6.0, unless there's a critical error that cannot be worked around.

ericoporto commented 3 weeks ago

@Crystal-Shard can you 100% reproduce this in the latest 3.6.1 Editor?

Crystal-Shard commented 3 weeks ago

Regarding the version, since 3.6.1 was released, I'm now not patching previous versions like 3.6.0, unless there's a critical error that cannot be worked around.

That's fine. I'm simply reporting bugs for the AGS version I'm currently using, I don't always keep pace with the latest builds. If an issue is important to me and has been fixed in a later engine, well that's a good reason for me to upgrade :)

ivan-mogilko commented 3 weeks ago

can you 100% reproduce this in the latest 3.6.1 Editor?

The problem reproduces in 3.6.1 using above steps, with the exception of editor failing to load. Game.agf keeps mentioning a non-existing translation file, and it gets recreated after loading a project.

ivan-mogilko commented 3 weeks ago

Yes. So my issue here is with the warning "If you don't save the game now, you may not be able to successfully open this game next time."

Instead of this warning, simply save the game. The game is also saved automatically when you compile, when you fill a TRS file, and probably in a few other situations. It's not really helpful to ask the user "If you don't do X, your game file will be unusable. Do you want to X?"

I gave this some thought, and, in regards to saving the project unconditionally, I would not take such step lightly. There are couple of considerations here:

  1. User may have done other changes to the project prior to adding or deleting translation, and does not want to save these changes. If we just save unconditionally, that will go against user's expectations and may damage their work. This is a rare case, but I see how it's possible. NOTE that editor does not save automatically when filling a translation, it actually warns the user that the game has to be saved in order to proceed.
  2. There are few actions that create new files in the project now, but I may imagine a hypothetical situation where adding anything to project corresponds to adding a file. This is a matter of asset management and storage, and may change from version to version. If we take as a principle, that whenever something adds a file, a game implicitly saves, then such principle may cause problems in the future. This may even lead to a question: do we have to ask and not just save on exit every time?
  3. Editor may be forcefully closed at any random moment (program crash, power shutdown, etc), so implicit saving on exit does not solve this problem to full extent anyway.

For this reason, I'd rather suggest going other way, not save without a question, but ensure that Editor:

  1. Can restore project's state on load if anything is missing.
  2. Can reimport previously created files if they got "detached" by not saving a project.
  3. Alternatively, can clean the new but "detached" project files up when not saving. In other words - treat these new files as temporary unless project is saved. NOTE: I know this this may be dangerous and lead to a loss of manual work too, so mark this as an option in some cases.
Crystal-Shard commented 3 weeks ago

Well it's true that saving unconditionally shouldn't be done lightly... but the editor already saves unconditionally whenever you build the game, or rebuild one of the translation sources, or probably in some other cases. So maybe it's not such a big deal to save unconditionally?

ivan-mogilko commented 3 weeks ago

I don't know what is the right way to deal with this just yet. I may be overthinking this case, but my head is occupied with other things now; and I'd like to discuss this with other developers first.

What should be certainly done in the meantime, is double check that Editor can recover the project if any auxiliary files are missing, e.g. by creating a dummy (empty) replacement.