EtienneLamoureux / TQVaultAE

Extra bank space for Titan Quest Anniversary Edition
MIT License
288 stars 62 forks source link

Why change the File format? #436

Closed Malgardian closed 1 year ago

Malgardian commented 2 years ago

Hi, TQCollector dev and old time TQVault contributor here.

I got alerted to the recent change of TQVault's file format by an issue raised on my own project, so I decided to come here to ask the following things:

  1. What was the reason for introducing a new file format after 14+ years of TQVault's old file format?
  2. Would you consider making this an optional change, i.e. provide a compatibilty option (settings option, etc.)?
  3. If that is a net positive change and it's here to stay, would you at least be willing to provide documentation for the new file format?

I'll freely admit that this (to my uninformed mind) unnecessary change annoys me a bit. You're obviously under no obligation to give a headsup to other projects working in tandem with TQVault, but it would've still been a great gesture to at least announce such a breaking change to what has been a stable file format for many years. I'm not set in my ways. While keeping the old file format is still my preferred outcome, I'm willing to listen and be convinced about the advantages of the new file format.

Cheers, Mal

hguy commented 2 years ago

Hi @Malgardian,

All the persistence parts of this PR #421 has been possible "IN MINUTES" because of this change. The 14 years old binary format could not be extended because the code for vault read/write was the same as the game caravan (and it's a pain). Convenient solution at the time but cripple extensibility and flexibility of TVVaultAE over time. It could have be done other way for sure, but TQVaultAE files doesn't have to be shared with the game, therefore being stuck 14 years in the past was not necessary (in a TQVaultAE -> Game relation).

That PR has been in review mode for weeks and didn't raise any warnings. Does TQVault devs should have warn you or should you have been interested by TQVault live cycle ? (Github Notifications is one click away). TQCollector looks dependant of what's happening in TQVault, not the other way. Being a TQVault contributor doesn't imply being a TQCollector contributor.

Regarding the ability to read/write the new format, it's quite easy.

// Read
TQVaultAE.Data.PlayerCollectionProvider.ParseJsonData(PlayerCollection pc, string path);

// Write
TQVaultAE.Data.PlayerCollectionProvider.SaveVaultAsJson(PlayerCollection pc, string fileName);

// Data object models : TQVaultAE.Data.Dto.*

I don't understand why TQVault contribs should try to convince you about anything when they push a PR (you obviously don't look at them), but that being said, #421 should be self explanatory and from a dev perspective, it was priceless.

Cheers, hguy

Malgardian commented 2 years ago

All the persistence parts of this PR #421 has been possible "IN MINUTES" because of this change. The 14 years old binary format could not be extended because the code for vault read/write was the same as the game caravan (and it's a pain). Convenient solution at the time but cripple extensibility and flexibility of TVVaultAE over time. It could have be done other way for sure, but TQVaultAE files doesn't have to be shared with the game, therefore being stuck 14 years in the past was not necessary (in a TQVaultAE -> Game relation).

Fair enough. I personally don't see the need for those persistent features to be implemented that particular way, but that's totally fine. I can see how they can be neat features for others.

Out of curiosity (and maybe it's totally obvious to everyone but me) why not track those persistant things in a central settings file for all vault files instead? Actually interested, not trying to be an ass.

That PR has been in review mode for weeks and didn't raise any warnings. Does TQVault devs should have warn you or should you have been interested by TQVault live cycle ? (Github Notifications is one click away). TQCollector looks dependant of what's happening in TQVault, not the other way. Being a TQVault contributor doesn't imply being a TQCollector contributor.

No, as I said in my initial post I did not expect to be personally notified at all. I also, however, didn't expect such a big change to happen to TQVault or I would have indeed paid attention to its commits and PRs. I'm also well aware of what's dependent on what, yes.

I don't understand why TQVault contribs should try to convince you about anything when they push a PR (you obviously don't look at them), but that being said, #421 should be self explanatory and from a dev perspective, it was priceless.

No need to be like that. As I said above, I did not expect such a change to happen.

Thanks for the link to the PR, I'll have a look at it when I next have some days off from work.

P.S.: Also just to be clear, so it doesn't get lost in both translation and a text-only medium: I have no ill will towards you or your vision for TQVault. I was a bit annoyed at the additional workload for me, yes, but please don't consider that a personal attack.

hguy commented 2 years ago

Out of curiosity (and maybe it's totally obvious to everyone but me) why not track those persistant things in a central settings file for all vault files instead? Actually interested, not trying to be an ass.

Having a secondary persisted collection of vault data and tracking all the CRUD event related to the main collection into that second collection looked like extra work + extra code + extra test time. I did noticed that adding properties to PlayerCollection was enough and the path to "less changes" in the code base.

Question : how to include these new properties into binary file structure ? Answer : you can't because it's shared with game file and you don't want to even try to include conditional saving in the mix (that could have broken your reader too) ! Question : what's the easy to use, universaly understood, data format these days ? Answer : JSON Question : Is it hard to bypass Vault saving into JSON Answer : No! few lines of code limited to PlayerCollectionProvider and 3 classes for file structure (DTO). Question : What's the gain ? Answer : JSON advantages

I was convinced at the first point already!

P.S.: Also just to be clear, so it doesn't get lost in both translation and a text-only medium: I have no ill will towards you or your vision for TQVault.

I don't have "a vision" for TQVault. I'm just a player and i need that tool to enhance my play time and bring solutions to my player problems. It just happened that i'm competent enough to update the tool by myself and share these modifications with everybody.

I was a bit annoyed at the additional workload for me, yes, but please don't consider that a personal attack.

I was a bit annoyed that you ask us, in such a condescending way, to do extra work because you didn't want to do any.

Harry37x2 commented 1 year ago

Ok so... Is it possible to convert old .vault files to .json ? :> If so, how to ? My Vault is from 2017.

hguy commented 1 year ago

Hi @Harry37x2 You just need to upgrade to last TQVaultAE then select your vault in the tool. It will convert your .vault files into a .vault.json file. The old .vault files are still there but obsolete from now on.