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 options in xml format #1704

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

save options in xml format #1704

wants to merge 29 commits into from

Conversation

kianzarrin
Copy link
Collaborator

@kianzarrin kianzarrin commented Dec 2, 2022

fixes #562

saved game options are serialized as xml and then converted to byte[] data.

  • paves the path for setting options as default for new games.
  • Also moved out SavedGamePathFinderEdition because it will it does not belong there and would make it difficult to set options as default for new games.
  • Kept legacy code for loading options but there is no need for the legacy save code.
  • bumped Config version

TMPE.zip

@kianzarrin kianzarrin added Settings Road config, mod options, config xml serialization load/store data in memory or on disk labels Dec 2, 2022
@kianzarrin kianzarrin self-assigned this Dec 2, 2022
@kianzarrin kianzarrin added the Dependent This issue is blocked by another issue. label Dec 2, 2022
@kianzarrin kianzarrin marked this pull request as ready for review December 2, 2022 16:28
@kianzarrin kianzarrin linked an issue Dec 2, 2022 that may be closed by this pull request
@kianzarrin kianzarrin added the Blocking Another issue depends on this issue. label Dec 2, 2022
TLM/TLM/State/SavedGameOptions.cs Outdated Show resolved Hide resolved
TLM/TLM/State/SavedGameOptions.cs Outdated Show resolved Hide resolved
@kvakvs
Copy link
Collaborator

kvakvs commented Dec 5, 2022

I see legacy save is deleted - so this will upgrade legacy savefiles to XML and no going back?
Do we ever need users to be able to go back? Probably its safe to say they don't need to load saves in old TM:PE

@krzychu124
Copy link
Member

I see legacy save is deleted - so this will upgrade legacy savefiles to XML and no going back? Do we ever need users to be able to go back? Probably its safe to say they don't need to load saves in old TM:PE

IMO, it's not the best idea but not so bad either. Depends on how it will be released.
It can be done similar like how deprecation in software is done. As an example, first something is marked as deprecated and then removed at some point in the future, The important note is that deprecated thing still works so in our case we should still save data the old way additionally just to be able to switch back in case of problems.
Not too long, maybe just for one version, but still.
Remember, we have Test and Stable, recently they were released in sync (because of the game update required compatible version of mod) but as of migration it might be better to test things first to at least see if everything works in more random conditions, but also... can be released again like recently - in sync, so basically no way of going back.

Upcoming game update (Dec 13, 2022) doesn't break anything in the mod (as opposed to previous patches) so theoretically user could use previous version but if I release both Test and Stable with the same version they won't have a chance to do so 😂
Anyways, I'll try to retest this today again and we will see.

@kianzarrin
Copy link
Collaborator Author

@krzychu124 @kvakvs to make it forward compatible I need to first save legacy and then save xml and then place it in the byte array.
loading needs to skip over the legacy part.

its not hard but could be a bit confusing and annoying to read.

should I go ahead and do it?

@kvakvs
Copy link
Collaborator

kvakvs commented Dec 5, 2022

@krzychu124 @kvakvs to make it forward compatible I need to first save legacy and then save xml and then place it in the byte array. loading needs to skip over the legacy part.
its not hard but could be a bit confusing and annoying to read.
should I go ahead and do it?

Wait for Krzychu's opinion, i think we do not need dual save format. Is that a realistic scenario when users from Epic games/where older game version is running, try and load new XML-only savegames?

@kianzarrin
Copy link
Collaborator Author

Is that a realistic scenario when users from Epic games/where older game version is running, try and load new XML-only savegames?

Legacy load is supported.

@kianzarrin kianzarrin added this to the 11.7.3.0 milestone Dec 5, 2022
@kvakvs
Copy link
Collaborator

kvakvs commented Dec 5, 2022

Is that a realistic scenario when users from Epic games/where older game version is running, try and load new XML-only savegames?

Legacy load is supported.

Legacy save is deleted, i wonder if that is a very rare scenario when it is necessary and we can ignore it.

@krzychu124 krzychu124 self-requested a review December 8, 2022 21:57
Copy link
Member

@krzychu124 krzychu124 left a comment

Choose a reason for hiding this comment

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

I don't think you've tested the fix. It doesn't work for me, it's loading previous version (legacy), but after hot-reload it somehow loads the correct settings, so I have no idea what is going on. Maybe UI was build before the settings? I don't think so, since it wouldn't work before the change either.

@krzychu124
Copy link
Member

krzychu124 commented Dec 8, 2022

So.. we partially need #1709 because of changes you've made in #1702, now the UI is not updated when value changes.
Previously UI components were loading settings updating UI state but also setting the Options values which probably is not the case currently

On Hot-reload order of calls (read settings, init Settings screen) is different -> first load settings, then generate SettingsUI

@kianzarrin kianzarrin marked this pull request as draft December 13, 2022 01:35
@kianzarrin
Copy link
Collaborator Author

Convert to draft because I have not tested my fix.

@kianzarrin
Copy link
Collaborator Author

@krzychu124 do I have to use IObserver/IObservable? or the current approach is fine?

@krzychu124 krzychu124 modified the milestones: 11.7.3.0, 11.7.4.0 Dec 13, 2022
Base automatically changed from OptionsInstance to master December 16, 2022 16:14
@kianzarrin kianzarrin marked this pull request as ready for review December 16, 2022 16:23
@kianzarrin kianzarrin marked this pull request as draft December 16, 2022 19:43
@kianzarrin kianzarrin marked this pull request as ready for review December 16, 2022 19:45
if (!OptionsManager.Instance.LoadData(options ?? new byte[0])) {
loadingSucceeded = false;
if (!TMPELifecycle.IsNewGame) {
if (Version <= 3) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a bug, or you should swap bodies because LoadData(data) is loading XML Options (OPTIONS_ID)
which does not exist if version is equal or lower than 3

@krzychu124 krzychu124 modified the milestones: 11.7.4.0, Planned stuff Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocking Another issue depends on this issue. Dependent This issue is blocked by another issue. serialization load/store data in memory or on disk Settings Road config, mod options, config xml
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use XML to save/load options.
3 participants