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

Implement proper types for SaveReader which match Palworld repr #16

Open
wojiaolw322 opened this issue May 24, 2024 · 1 comment
Open
Labels
enhancement New feature or request

Comments

@wojiaolw322
Copy link

https://github.com/CrystalFerrai/UeSaveGame/
https://github.com/xkp95175333/palworld-sdk/blob/main/SDK/Pal_structs.hpp

@tylercamp tylercamp changed the title Suggest type references for reader Implement proper types for SaveReader which match Palworld repr May 24, 2024
@tylercamp
Copy link
Owner

On my phone and on break so I haven't looked deeply into the links yet, but my initial thoughts:

I considered writing full C# types which would map to the GVAS content, rather than dynamically collecting the structure at runtime, which would make it much easier to work with the parsed data

My only concerns:

  1. The current dynamic approach with visitor pattern helps to minimize memory usage by collecting values instead of parsing the whole structure. However, your suggestion doesn't necessarily require parsing the whole structure and can still just parse what we need, so overall memory usage would still be the same (if not better + reduced runtime due to visitor overhead)
  2. I wanted to keep my save reader impl as close to the reference as possible, and adding proper types would move away from that, but if there are other references I can use which already have proper types then that shouldn't be an issue. (Keeping impls close to reference makes it easier to apply updates from the reference)
  3. Proper typing sort of implies stricter parsing and sensitivity to minor changes in the data format. That only applies if we write purely-binary readers though. The properties in the GVAS are still labeled with strings, so strict binary parsing isn't necessarily required

This broader goal is definitely something to consider and would make save-related bugs easier to troubleshoot. My original concerns seem largely moot.

I'm just not sure about the effort required to bring all of those types in

@tylercamp tylercamp added the enhancement New feature or request label May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants