Skip to content

Conversation

@HeroponRikiBestest
Copy link
Contributor

This PR is a draft until I do another pass for proper formatting and commenting, as well as more testing, and other requirements that need to be fulfilled.

@HeroponRikiBestest
Copy link
Contributor Author

PR is ready for review. Let me know if there's anything I still need to do, this should be fully complete.

HeroponRikiBestest and others added 18 commits January 22, 2026 21:23
…memory (SabreTools#56)

* WIP

* WIP 2

* Todo: you're missing a read somehow and getting misaligned by two bytes? maybe properly implementing the buffer will magically fix it

* continued blocks my behated

* Pre-major-testing

* Forgot to add summaries

* Attempt to properly roll back. The state i wanted to roll back to wasn't in a commit before.

* Figured out the issue with the rolled back commit, this has to be a while loop because of 0 byte files. Reimplemented clean code.

* Comment so I don't forget why it's like this

* Skip unsupported compression types before opening filestream.

* Reenable non-start cab skipping

* TODO so i don't forget

* TODO so I don't forget

* iterate on continued block correctly.

* Handle incomplete extraction better

* Remove TODOs to ready for PR comments

* Missed one

* Some minor fixes before rewriting everything

* Next round of fixes.

* Next set of fixes

* Fix debug output
… methods (SabreTools#57)

* Attempt 1

* Use private helper class

* Remove comment

* Add missing summaries

* Don't use create pattern

* Remove removed variable summary

* Reduce what needs to be passed in

* Further reduce what needs to be passed in

* These didn't get tabbed for some reason

* First round of fixes.

* Second round of changes

* Add a few comments.
* Re-enable extraction from stream

* Remove todo pre PR

* Properly support all situations, remove whitespace
…stead of living in PortableExecutable (SabreTools#59)

* Figure out how to access OverlayAddress in wrapper or reader (ideally the latter) for a non-PE reader/wrapper

* Code works

* Remove TODOs

* First round of fixes.

* use constants

* remove comment
I had previously been under the assumption that closing always guaranteed a flush. I don't know of any issues that were being caused, but I should push this before i forget.
Copy link
Contributor

@mnadareski mnadareski left a comment

Choose a reason for hiding this comment

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

I'm a bit confused with the naming here. I'm seeing both SkuSis and VDF being used seemingly interchangeably. Is there a difference here in what both mean? Is one preferable over the other? Or is this a subset situation like COFF and PE?

/// <summary>
/// A JSON Object representing the VDF structure.
/// </summary>
public JObject? VDFObject { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure this is a complicated question, but why does this have to be a JObject and not another structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What other structures would you use? I'm open to others if you have any specific suggestions, JObject was just what I happened to find worked best out of all the various JSON-based options.

Comment on lines +1 to +7
using System;
using System.IO;
using SabreTools.IO.Extensions;
using File = SabreTools.Data.Models.VDF.File;
using static SabreTools.Data.Models.VDF.Constants;
using System.Text;
using Newtonsoft.Json.Linq;
Copy link
Contributor

Choose a reason for hiding this comment

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

Ensure usings are ordered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also just for future reference- how do

using File = SabreTools.Data.Models.VDF.File;
using static SabreTools.Data.Models.VDF.Constants;

get alphabetized?
Do I

  • alphabetize based on the first letter no matter what
  • alphabetize based on the name of the actual package (in this case SabreTools.Data.whatever)
  • put them after all the "normal" using statements

Comment on lines 42 to 46
if (skuSis == null)
return null;

if (skuSis.VDFObject == null)
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

If both are required, you can replace this by a if (skuSis?.VDFObject == null) check instead.

Comment on lines 68 to 69
string json = "{\n"; // Sku sis files have no surrounding curly braces, which json doesn't allow
string delimiter = "\"\t\t\""; // KVPs are always quoted, and are delimited by two tabs
Copy link
Contributor

Choose a reason for hiding this comment

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

These two would likely benefit from being const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

json is the string that gets updated, so that one can't be const, unless you want me to copy it.


string json = "{\n"; // Sku sis files have no surrounding curly braces, which json doesn't allow
string delimiter = "\"\t\t\""; // KVPs are always quoted, and are delimited by two tabs
string? line;
Copy link
Contributor

Choose a reason for hiding this comment

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

If line is never used outside of the loop, please instantiate it in the loop.


#region SkuSis

// TODO: add description
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: please add description

Copy link
Contributor Author

@HeroponRikiBestest HeroponRikiBestest Jan 27, 2026

Choose a reason for hiding this comment

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

Weird, I thought I fixed this already, I specifically went through all my TODOs before undrafting. Not sure why this is still here.
That said, I'm also not sure why I said this? Very few of the other regions have descriptions. Do you have any idea why it's there?

// TODO: add description
if (magic.StartsWith(Data.Models.VDF.Constants.SteamSimSidSisSignatureBytes)
|| magic.StartsWith(Data.Models.VDF.Constants.SteamCsmCsdSisSignatureBytes))
return WrapperType.SkuSis;
Copy link
Contributor

Choose a reason for hiding this comment

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

Curly braces please

HeroponRikiBestest and others added 2 commits January 27, 2026 15:52
…memory (SabreTools#56)

* WIP

* WIP 2

* Todo: you're missing a read somehow and getting misaligned by two bytes? maybe properly implementing the buffer will magically fix it

* continued blocks my behated

* Pre-major-testing

* Forgot to add summaries

* Attempt to properly roll back. The state i wanted to roll back to wasn't in a commit before.

* Figured out the issue with the rolled back commit, this has to be a while loop because of 0 byte files. Reimplemented clean code.

* Comment so I don't forget why it's like this

* Skip unsupported compression types before opening filestream.

* Reenable non-start cab skipping

* TODO so i don't forget

* TODO so I don't forget

* iterate on continued block correctly.

* Handle incomplete extraction better

* Remove TODOs to ready for PR comments

* Missed one

* Some minor fixes before rewriting everything

* Next round of fixes.

* Next set of fixes

* Fix debug output
mnadareski and others added 14 commits January 27, 2026 15:52
* Re-enable extraction from stream

* Remove todo pre PR

* Properly support all situations, remove whitespace
…stead of living in PortableExecutable (SabreTools#59)

* Figure out how to access OverlayAddress in wrapper or reader (ideally the latter) for a non-PE reader/wrapper

* Code works

* Remove TODOs

* First round of fixes.

* use constants

* remove comment
I had previously been under the assumption that closing always guaranteed a flush. I don't know of any issues that were being caused, but I should push this before i forget.
@HeroponRikiBestest
Copy link
Contributor Author

I'm a bit confused with the naming here. I'm seeing both SkuSis and VDF being used seemingly interchangeably. Is there a difference here in what both mean? Is one preferable over the other? Or is this a subset situation like COFF and PE?

See the explanation at the top of the Readers file.

https://github.com/SabreTools/SabreTools.Serialization/pull/54/changes#diff-54b774d3290e04aa399006ebbba19aa9a72475efc8265e4057e4d9fe317b0e22R11-R18

@HeroponRikiBestest
Copy link
Contributor Author

closing and reopening because apparently i don't know how to merge

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

Successfully merging this pull request may close these issues.

2 participants