Some serialization cleanup, optimization, fixes #1864
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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
reviewed
Aug 22, 2024
| @@ -21,10 +21,8 @@ public long Position | |||
| get => Stream.Position; | |||
| set | |||
| { | |||
| #if DEBUG | |||
| if (value > Length) | |||
| if (value < 0 || value > Length) | |||
There was a problem hiding this comment.
nit, can be changed into a pattern
| @@ -86,10 +86,8 @@ public long AbsPosition | |||
| { | |||
| if (isUsingBufferReader) | |||
| { | |||
| #if DEBUG | |||
| if (value > Length) | |||
| if (value < 0 || value > Length) | |||
| @@ -110,6 +110,8 @@ Opcode.PushBltn or Opcode.PushI | |||
| _ => throw new IOException("Unknown opcode " + op.ToString().ToUpper(CultureInfo.InvariantCulture)), | |||
| }; | |||
| } | |||
|
|
|||
| // 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.
|
|
||
| case InstructionType.PopInstruction: | ||
| { | ||
| if (Type1 == DataType.Int16) | ||
| { | ||
| // 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.
Suggested change
| // 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.
Suggested change
| // 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) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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