Some serialization cleanup, optimization, fixes#1864
Some serialization cleanup, optimization, fixes#1864colinator27 wants to merge 3 commits intomasterfrom
Conversation
- Removed more cases of `#if DEBUG`, to ensure consistent behavior on Release builds. - Added more safety checks to binary readers, and to deserialization in general (preventing unintended negative lengths/jumps that could lead to infinite loops). - Fixed `EmSize` on fonts (when in float form) being accidentally always parsed as 0, due to a typo. - Completely refactored code instruction deserialization, removing many calls and I/O operations - Commented and reformatted code instruction deserialization/serialization
|
Download the artifacts for this pull request here: GUI:
CLI: |
Seems like it was never there
|
The |
|
the emsize fix should be removed from here now |
Miepee
left a comment
Miepee
left a comment
There was a problem hiding this comment.
Only reviewed the changes in UndertaleCode so you have something to work with.
Please split the PR up into the following
- #if debug changes
- code commenting in UTcode serialization
- code refactor in UTcode deserialization
- binary reader checks
- QOI padding fix.
| { | ||
| #if DEBUG | ||
| if (value > Length) | ||
| if (value < 0 || value > Length) |
There was a problem hiding this comment.
nit, can be changed into a pattern
| { | ||
| #if DEBUG | ||
| if (value > Length) | ||
| if (value < 0 || value > Length) |
| }; | ||
| } | ||
|
|
||
| // Converts from bytecode 14 instruction opcodes to modern opcodes |
| else | ||
| { | ||
| // Bytecode 15 and above encode a comparison kind outside of the opcode | ||
| writer.Write((byte)ComparisonKind); |
There was a problem hiding this comment.
mental note: check where comparisonKind is all used, and potentially make it a byte enum
unsure if thats addressed by underanalyzer.
| { | ||
| // Special scenario - the swap instruction | ||
| // TODO: Figure out the proper syntax, see #129 | ||
| // Special scenario - the swap instruction (see #129) |
There was a problem hiding this comment.
ideally we'd have a proper wiki for this than linking to issues lol
| @@ -382,22 +384,39 @@ public Reference<T> GetReference<T>(bool allowResolve = false) where T : class, | |||
| /// <inheritdoc /> | |||
| public void Serialize(UndertaleWriter writer) | |||
There was a problem hiding this comment.
a more general note, but with how many small writing operations are done, i wonder if it makes sense to buffer them somewhere and then write them later all at once.
| { | ||
| throw new IOException($"Invalid padding in {kind.ToString().ToUpper(CultureInfo.InvariantCulture)}"); | ||
| } | ||
| if (instructionType == InstructionType.SingleTypeInstruction && type2 != 0) |
| } | ||
| } | ||
|
|
||
| // Cheeck for "and.b.b" or "or.b.b", which imply the code was compiled without short-circuiting |
There was a problem hiding this comment.
| // Cheeck for "and.b.b" or "or.b.b", which imply the code was compiled without short-circuiting | |
| // Check for "and.b.b" or "or.b.b", which imply the code was compiled without short-circuiting |
| if (JumpOffset == -1048576) // magic? encoded in little endian as 00 00 F0, which is like below | ||
| JumpOffsetPopenvExitMagic = true; | ||
| reader.Position++; | ||
| // Bytecode 14 has slightly simplified parsing |
There was a problem hiding this comment.
| // Bytecode 14 has slightly simplified parsing | |
| // Bytecode 14 has slightly different parsing |
| // TODO: Figure out the proper syntax, see #129 | ||
| SwapExtra = (ushort)TypeInst; | ||
| TypeInst = 0; | ||
| // Special scenario - the swap instruction (see #129) |
3 participants
Description
#if DEBUG, to ensure consistent behavior on Release builds.FixedEmSizeon fonts (when in float form) being accidentally always parsed as 0, due to a typo.Caveats
Has potential to break some instruction parsing if I typo'd somewhere, but I did test it on multiple games (including Undertale 1.00, a bytecode 14 game), and it seems to work fine. If it does break, it'll be pretty obvious, as re-saving a data.win file will show differences in the code.
Notes
Probably too risky to merge into stable,
although in the event that we make a minor release, we may want to backport the. It affects the Deltarune Chapter 1 & 2 demo, version 1.15 (which I tested this PR on).EmSizetypo fix, as that is pretty diabolical