3.6.1: Remove AlignedStream

[ivan-mogilko]

This is a remake of #2193 for the backwards compatible 3.* branch.

AlignedStream was used initially in attempt to automate reading legacy formats, where structs were dumped from memory into filestream directly, resulting in alignment padding inserted into the file format (according to 32-bit system alignment, where first AGS games were created).

While using AlignedStream made working with old formats easier, this approach also has certain problems:

1. This makes extra padding in file formats implicit and non-obvious.
2. Object Read/Write functions will depend on the type of the passed stream.
3. Adding anything into Read/Write functions will also be affected by AlignedStream, and in theory may lead to additional unnecessary padding inserted simply by keeping same serialization logic (I think this never really happened to the game files, fortunately, but could have. OTOH it might have happened to legacy saves, this is something that needs separate investigation.)
4. AlignedStream does not support Seek, and in general you cannot trivially skip parts of struct by skipping or reading N bytes, you'll have to read it out step by step to keep alignment calculation working.

These concerns made me make the decision to remove AlignedStream, and restore hardcoded padding instead. It's explicit and honest.

This PR is currently a draft, 3.* branch contains more cases of padded data in files, and in legacy save format too, so this will take an extra care to do properly, and test corresponding game versions.

• - Games with legacy Interactions & Command lists;
• - Games with ViewStruct272 data;
• - Games with legacy saves (3.2.1 - 3.4.0);
• - Games with saved property values in legacy saves (3.4.0);
• - Games with animated buttons in legacy saves.
• - Games with Overlays in legacy saves.

[ericoporto]

I feel that for things used in new save file format and new things this is a good move! Looking at the code changes, I am curious if it would be a good idea for future save formats to have some strategy towards strings, like saving the length of the string, because that manual padding after strings (like in the case of audio clips file name and script name) looks super fragile.

[ivan-mogilko]

AlignedStream was never used in new save format, it was used in old data files, and old saves (prior to 3.5.0). Contemporary data files still have the padding in some sections because they never changed, but new save format was rewritten from scratch, so it does not have any.

In regards to strings, these paddings are not related to how these strings are written, with their length or not, in any way; it's all related to how structs were positioned in memory in the old engine times.

This whole aligned padding story was because in the old engine some structs were dumped into file directly from memory, like fwrite(&object);, which resulted in memory padding, and even pointers and virtual table pointers saved as meaningless values.

[ericoporto]

Contemporary data files still have the padding in some sections because they never changed

I think I confused with the gamesetupstruct, which I think is the thing that initializes the game. I was trying to understand the audio clip comment about padding to int16 with a single read8.

I remember when writing tests for aligned stream that it broke in win64 builds, I don't know if there's anything specifically that is required to check.

[ivan-mogilko]

This padding depends on existing format which was written in file, that does not depend on which system this runs on.
AlignedStream itself, or tests perhaps could depend on a size_t or pointer size, but that's a separate issue.

The rules of padding was mentioned in comments to Alignment stream, but from my memory, the idea is to pad when smaller types are followed by larger types, and the number of written smaller types is odd.

example 1:

• int8
• int8
• int16

Here 2 x int8 result in 1 x int16, so no padding is necessary

example 2:

• int8
• int8
• int8
• int16

Here 3 x int8 result in 1.5 x int16, so we have to insert another int8 to round things up before real int16 is read.

Same happens when int32 appears after int8 and int16. Same would happen with int64, but AGS never had these in its structs.

All pointer values that have been unnecessarily written into data formats were 32-bit, because prior to open source AGS was strictly a 32-bit program.

[ivan-mogilko]

Mostly done, only testing remained (according to the todo checklist in the ticket).

Also, I think I will wait merging this until after another 3.6.1 Beta update, since there were some fixes that I'd rather not mix with this serious change in code.

The changes in this PR will affect 3 things:

1. Parts of the current game data. But this is easy to test, as anything wrong there should be noticed quickly. Also, only reading changed, writing (done by editor) remains intact, so nothing should get corrupted.
2. Parts of the legacy game data.
3. Legacy save format (pre-3.5.0).

In other words, most concern here is in loading old games and old saves.

[ericoporto]

I think a test that can be done is to load an old save and save in the current version, and then do the same in this PR version. The later generated save file should match exactly - an md5sum check can be run in both files to verify their contents are equal.

[ivan-mogilko]

I think a test that can be done is to load an old save and save in the current version, and then do the same in this PR version. The later generated save file should match exactly - an md5sum check can be run in both files to verify their contents are equal.

Which purpose would this test achieve? The contents of a save don't depend on format that was restored before them, and contemporary saves should not be affected by this PR at all, as they never used AlignedStream.

EDIT: Oh, I probably understand what you mean.
But for this one would have to take a game that does not run anything in rep-exec, or hardcode saving a new game right after loading old one in the engine in order to prevent things changing between load and save.

[ivan-mogilko]

Tested everything I could remember, found and fixed 1 typo, but this seems work overall.
Had to rebase after finding a mistake with loading overlays (unrelated): 4bbc422

Tested that saves made with this pr may be loaded by the 3.6.1, and vice-versa.