Fix creation of IPS patches with records beginning at 454F46 #314
Conversation
This file contains hidden or 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
charasyn
approved these changes
Feb 9, 2025
Contributor
charasyn
left a comment
charasyn
left a comment
There was a problem hiding this comment.
Thanks for making this PR!
As discovered by @Messianic, this code path was buggy and not properly tested. It made the record a single int instead of a bytes object.
PhoenixBound
force-pushed
the
patch-eof-offset
branch
from
96d9e2b to
389fb17
Compare
ichee
added a commit
to ichee/CoilSnake
that referenced
this pull request
Feb 10, 2025
* Fix creation of IPS patches with records beginning at 454F46 (pk-hack#314) As discovered by @Messianic, this code path was buggy and not properly tested. It made the record a single int instead of a bytes object. * Upgrade project files * Update EnemyModule.py * Upgrade project files * Update EnemyModule.py --------- Co-authored-by: PhoenixBound <PhoenixBound@users.noreply.github.com>
ichee
added a commit
to ichee/CoilSnake
that referenced
this pull request
Feb 10, 2025
* Fix creation of IPS patches with records beginning at 454F46 (pk-hack#314) As discovered by @Messianic, this code path was buggy and not properly tested. It made the record a single int instead of a bytes object. * Upgrade project files * Update EnemyModule.py * Upgrade project files * Update EnemyModule.py * Update version maybe * Update EnemyModule take 2 * Update eb.yml * Update EnemyModule.py --------- Co-authored-by: PhoenixBound <PhoenixBound@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
When a ROM has a change that begins at address 454F46 (ASCII
EOF), it can't normally be encoded in an IPS file, because that offset value is used to mark the end of the patch. CoilSnake did have code to catch that case, but it seems like it was never tested, at least not in any recent version of Python... it caused a singleintto be inserted as the change to make to an offset, rather than abytesobject. All of the other code was based onbytesobjects being appended together, so the next loop iteration would raise a TypeError (unsupported operand type(s) for +=: 'int' and 'bytes')Changes were tested by changing an all-zeroes file to have the bytes
DE ADat offset 454F46 and confirming that the IPS patch contained a 3-byte change beginning at offset 454F45 consisting of00 DE AD.The design of this patch creation loop is... kinda weird, with how it processes -1 or 0 or 1 bytes each loop iteration (this PR fixes the only -1 case), and how it manually calls methods like
__len__()and__getitem__(index). But it doesn't seem buggy apart from this one thing.