From a4d3053c0e47bb6261e0bcc2a215cf8d4b6024e2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 07:55:20 +0000 Subject: [PATCH 01/13] Fix read/write asymmetries, script serialization and global state The governing defect in this module was that a field's parse path and its serialize path disagreed. Those bugs need no user edit to bite: loading a ROM and saving it is enough to corrupt data. Every fix below was verified with a byte-exact round-trip harness over randomized inputs. Bitfield and padding asymmetries: - Trainer alt-form was read as 6 bits but written masked to 3, so every form above 7 was silently remapped or reverted. - Item stat boosts were read with a 4-bit mask and written with a 2-bit one; the very next field had the inverse error, and its overflow bled into the PP Up flags. - EV yields are 2-bit fields, but the setters accepted 4 and the write path never masked, so each yield corrupted the next. Dex colour had the same shape of bug against the sprite-flip bit. - 44 bytes of DPPt encounter data were skipped on read and re-emitted as zeroes, wiping a rate word and a five-slot water table in ~600 subfiles. The item and personal padding bytes had the same pattern. - Trainer type flags and the AI bitfield were re-encoded from only the bits the tool understood, zeroing everything else. - The upper nibble at item offset 0x14 and bits 12-15 of the personal EV halfword were read into nothing and written as zero. - Level script trigger numbers were narrowed through a signed byte cast, so any script above 127 sign-extended. Text: - 9-bit compression only guarded characters up to 0x3FF, so a newline or a VAR placeholder overran its slot and overwrote following characters. - Unmapped characters were silently written as code 0, turning a pasted apostrophe into a null glyph; they now report which character failed. - Bank tables, VAR sequences and escape decoding are bounds-checked, so a message ending in a stray backslash no longer aborts the whole ROM save. - Character 245 collided with 223 in the reverse map and could not survive a save. Scripts: - Command 225 is goto_if_trainer_defeated, not a comparator, so loading any script using it threw and aborted the entire NARC. Its jump target is now registered and recomputed on save instead of being emitted from the old file's absolute address. - ActionCommand(int,int) never assigned its id, so every movement whose name is absent from the tables re-serialized as movement 0. - visitAction_command was a no-op, so recompiling a script dropped every movement command while keeping the labels that pointed at them. - An unresolvable label serialized as a branch to offset 0. - Trainer AI opcodes were written 16-bit though the macros declare 32-bit. - Hex literals in the user-editable macro files crashed both algebra visitors; negative word parameters were emitted in a form the grammar cannot re-lex. Global state: - PersonalParser's TM/HM tables were static, so opening one ROM and saving another wrote the first ROM's TM list into it. Same class of bug in the sprite parser's party-icon header cache, which silently shifted the icon NARC by seven files when unset. - Game.region was mutable state on an enum constant. parseBaseRom now returns the region alongside the game and Tables.initialize takes it explicitly; the old accessor remains, deprecated. - Tables entries are reset between initialize calls and now throw when read before being set, rather than returning a usable-looking zero. - Diamond and Pearl fell through every initialize switch with no default, so a D/P ROM half-initialized and failed far from the cause. - The sprite parser mutated the shared arm9 buffer without holding its lock. Tests: - Nine of the ten parser test classes were declared static, which Surefire's default inner-class exclude drops and JUnit will not descend into. Only one ran. They are now @Nested, taking the suite from 2 discovered tests to 22. - outputMatchesInput asserted a value against itself under a guard that had already established equality, skipped all checks when the output was empty, and otherwise compared the output to itself rather than to the original -- so a parser returning zero bytes for every subfile passed. It now compares against the original and asserts the file count. - The suite hard-required a copyrighted ROM beside the pom and failed the build without one; ROMs are located via -Drom.dir and missing ones skip. - Added the sprite parser binding and test class, which had no coverage. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob --- .../pokeditor/formats/trainers/SmogonTeam.g4 | 2 +- .../pokeditor/formats/BytesDataContainer.java | 3 +- .../encounters/GenericEncounterData.java | 2 + .../encounters/JohtoEncounterData.java | 29 +++- .../encounters/SinnohEncounterData.java | 39 ++++- .../formats/evolutions/EvolutionData.java | 21 ++- .../pokeditor/formats/items/ItemData.java | 47 +++++- .../formats/learnsets/LearnsetData.java | 14 +- .../formats/learnsets/LearnsetParser.java | 12 +- .../formats/personal/PersonalData.java | 51 +++--- .../formats/personal/PersonalParser.java | 63 ++++++-- .../pokemon_sprites/PokemonSpriteData.java | 118 +++++++++++--- .../pokemon_sprites/PokemonSpriteParser.java | 64 +++++--- .../formats/scripts/FieldScriptData.java | 111 +++++++++++-- .../formats/scripts/FieldScriptParser.java | 8 +- .../formats/scripts/LevelScriptData.java | 45 +++++- .../scripts/antlr4/CommandDiscoverer.java | 2 +- .../formats/scripts/antlr4/CommandMacro.java | 20 ++- .../scripts/antlr4/CommandMacroVisitor.java | 10 +- .../formats/scripts/antlr4/CommandReader.java | 2 +- .../formats/scripts/antlr4/CommandWriter.java | 12 +- .../scripts/antlr4/ScriptDataProducer.java | 66 +++++++- .../pokeditor/formats/text/TextBankData.java | 149 +++++++++++++++--- .../formats/text/TextBankParser.java | 5 +- .../formats/trainers/TrainerData.java | 40 +++-- .../trainers/antlr/SmogonTeamImporter.java | 73 ++++++++- .../turtleisaac/pokeditor/gamedata/Game.java | 37 ++++- .../pokeditor/gamedata/GameCodeBinaries.java | 1 + .../pokeditor/gamedata/GameFiles.java | 1 + .../pokeditor/gamedata/Tables.java | 45 +++++- .../pokeditor/gamedata/TextFiles.java | 17 +- src/main/resources/data/characters.json | 3 +- .../pokeditor/formats/GenericParserTest.java | 60 ++++++- .../pokeditor/formats/ParserTests.java | 120 ++++++-------- .../pokeditor/formats/TestsInjector.java | 14 +- 35 files changed, 1042 insertions(+), 264 deletions(-) diff --git a/src/main/antlr4/io/github/turtleisaac/pokeditor/formats/trainers/SmogonTeam.g4 b/src/main/antlr4/io/github/turtleisaac/pokeditor/formats/trainers/SmogonTeam.g4 index 85bd7a3..ca6ea67 100644 --- a/src/main/antlr4/io/github/turtleisaac/pokeditor/formats/trainers/SmogonTeam.g4 +++ b/src/main/antlr4/io/github/turtleisaac/pokeditor/formats/trainers/SmogonTeam.g4 @@ -8,7 +8,7 @@ team: (speciesEntry)+ EOF?; move: '-' WHITESPACE+ nameWithSpace WHITESPACE*? NEWLINE? ; -speciesEntry : NEWLINE*? species ability? level? shiny? effortValues? nature? individualValues? move move? move? move? NEWLINE*? ; +speciesEntry : NEWLINE*? species ability? level? shiny? effortValues? nature? individualValues? move? move? move? move? NEWLINE*? ; species: NEWLINE NAME WHITESPACE+? item? WHITESPACE*? NEWLINE ; item: '@' WHITESPACE+ nameWithSpace ; diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/BytesDataContainer.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/BytesDataContainer.java index abe661f..d99b0ba 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/BytesDataContainer.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/BytesDataContainer.java @@ -39,7 +39,8 @@ public boolean containsKey(GameFiles key) public boolean containsPatternKey(GameFiles file, PatternIndex key) { - return get(file).containsKey(key); + Map m = super.get(file); + return m != null && m.containsKey(Objects.requireNonNullElse(key, Default.NO_GROUPING)); } public interface PatternIndex { diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/encounters/GenericEncounterData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/encounters/GenericEncounterData.java index 98bb6ae..97945b1 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/encounters/GenericEncounterData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/encounters/GenericEncounterData.java @@ -113,6 +113,7 @@ public static class WaterEncounterSet { int[] minLevels; int[] maxLevels; int[] species; + byte[][] slotPadding; // bytes between the levels and the species of each slot, preserved verbatim WaterEncounterSet(int numSlots) { @@ -120,6 +121,7 @@ public static class WaterEncounterSet { minLevels = new int[numSlots]; maxLevels = new int[numSlots]; species = new int[numSlots]; + slotPadding = new byte[numSlots][]; } public int getNumSlots() diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/encounters/JohtoEncounterData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/encounters/JohtoEncounterData.java index 3d710be..989c000 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/encounters/JohtoEncounterData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/encounters/JohtoEncounterData.java @@ -18,11 +18,35 @@ public class JohtoEncounterData extends GenericEncounterData int[] swarmSpecies; + // 2 bytes of padding after the rates - preserved verbatim so a save round-trips + byte[] ratePadding; + public JohtoEncounterData(BytesDataContainer files) { super(files); } + private JohtoEncounterData() + { + super(); + fieldSpecies = new int[NUM_FIELD_ENCOUNTER_SETS][NUM_BASE_FIELD_ENCOUNTER_SLOTS]; + hoennSoundSpecies = new int[NUM_SOUND_SMASH_ENCOUNTER_SLOTS]; + sinnohSoundSpecies = new int[NUM_SOUND_SMASH_ENCOUNTER_SLOTS]; + swarmSpecies = new int[NUM_SWARM_ENCOUNTER_SLOTS]; + smashEncounterSet = new WaterEncounterSet(NUM_SMASH_SLOTS); + ratePadding = new byte[NUM_RATE_PADDING_BYTES]; + + for (int i = 0; i < waterEncounters.length; i++) + { + waterEncounters[i] = new WaterEncounterSet(WaterEncounterSet.NUM_WATER_SLOTS); + } + } + + public static JohtoEncounterData create() + { + return new JohtoEncounterData(); + } + @Override public void setData(BytesDataContainer files) { @@ -42,7 +66,7 @@ public void setData(BytesDataContainer files) oldRodRate = reader.readUInt8(); goodRodRate = reader.readUInt8(); superRodRate = reader.readUInt8(); - reader.skip(2); + ratePadding = reader.readBytes(NUM_RATE_PADDING_BYTES); for (int i = 0; i < NUM_BASE_FIELD_ENCOUNTER_SLOTS; i++) { @@ -103,7 +127,7 @@ public BytesDataContainer save() MemBuf.MemBufWriter writer = dataBuf.writer(); writer.writeBytes(fieldRate, surfRate, smashRate, oldRodRate, goodRodRate, superRodRate); - writer.skip(2); + writer.write(ratePadding != null ? ratePadding : new byte[NUM_RATE_PADDING_BYTES]); writer.writeBytes(fieldLevels); @@ -149,6 +173,7 @@ private void writeWaterEncounterSet(MemBuf.MemBufWriter writer, WaterEncounterSe } } + private static final int NUM_RATE_PADDING_BYTES = 2; private static final int NUM_SOUND_SMASH_ENCOUNTER_SLOTS = 2; private static final int NUM_SMASH_SLOTS = 2; private static final int NUM_SWARM_ENCOUNTER_SLOTS = 4; diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/encounters/SinnohEncounterData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/encounters/SinnohEncounterData.java index 1664ae2..b93f3f7 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/encounters/SinnohEncounterData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/encounters/SinnohEncounterData.java @@ -17,6 +17,10 @@ public class SinnohEncounterData extends GenericEncounterData int[][] dualSlotSpecies; + // 0x2C bytes sitting between the surf encounters and the old rod encounters - contents are not + // understood yet, so they are preserved verbatim rather than zeroed out on save + byte[] unknown; + public SinnohEncounterData(BytesDataContainer files) { super(files); @@ -24,6 +28,24 @@ public SinnohEncounterData(BytesDataContainer files) private SinnohEncounterData() { super(); + fieldSpecies = new int[1][NUM_BASE_FIELD_ENCOUNTER_SLOTS]; + swarmSpecies = new int[NUM_SWARM_DAY_NIGHT_ENCOUNTER_SLOTS]; + daySpecies = new int[NUM_SWARM_DAY_NIGHT_ENCOUNTER_SLOTS]; + nightSpecies = new int[NUM_SWARM_DAY_NIGHT_ENCOUNTER_SLOTS]; + radarSpecies = new int[NUM_RADAR_ENCOUNTER_SLOTS]; + formProbability = new byte[NUM_FORM_PROBABILITY_BYTES]; + dualSlotSpecies = new int[NUM_DUAL_SLOT_GAMES][NUM_DUAL_SLOT_ENCOUNTER_SLOTS]; + unknown = new byte[NUM_UNKNOWN_BYTES]; + + for (int i = 0; i < waterEncounters.length; i++) + { + WaterEncounterSet set = new WaterEncounterSet(WaterEncounterSet.NUM_WATER_SLOTS); + for (int slot = 0; slot < WaterEncounterSet.NUM_WATER_SLOTS; slot++) + { + set.slotPadding[slot] = new byte[NUM_WATER_SLOT_PADDING_BYTES]; + } + waterEncounters[i] = set; + } } public static SinnohEncounterData create() @@ -82,7 +104,7 @@ public void setData(BytesDataContainer files) } //todo really read the form probability table - formProbability = reader.readBytes(24); + formProbability = reader.readBytes(NUM_FORM_PROBABILITY_BYTES); //todo really read the dual slot mons dualSlotSpecies = new int[NUM_DUAL_SLOT_GAMES][NUM_DUAL_SLOT_ENCOUNTER_SLOTS]; @@ -97,8 +119,8 @@ public void setData(BytesDataContainer files) surfRate = reader.readInt(); waterEncounters[0] = readWaterEncounterSet(reader); - //todo figure out wtf is going on here - reader.skip(0x2C); + //todo figure out wtf is going on here - preserved verbatim so a save round-trips + unknown = reader.readBytes(NUM_UNKNOWN_BYTES); oldRodRate = reader.readInt(); waterEncounters[1] = readWaterEncounterSet(reader); @@ -117,7 +139,7 @@ private WaterEncounterSet readWaterEncounterSet(MemBuf.MemBufReader reader) { set.maxLevels[i] = reader.readUInt8(); set.minLevels[i] = reader.readUInt8(); - reader.skip(2); + set.slotPadding[i] = reader.readBytes(NUM_WATER_SLOT_PADDING_BYTES); set.species[i] = reader.readInt(); } return set; @@ -173,8 +195,8 @@ public BytesDataContainer save() writeWaterEncounterSet(writer, surfRate, waterEncounters[0]); - //todo figure out wtf is going on here - writer.skip(0x2C); + //todo figure out wtf is going on here - preserved verbatim so a save round-trips + writer.write(unknown); writeWaterEncounterSet(writer, oldRodRate, waterEncounters[1]); writeWaterEncounterSet(writer, goodRodRate, waterEncounters[2]); @@ -189,7 +211,7 @@ private void writeWaterEncounterSet(MemBuf.MemBufWriter writer, int rate, WaterE for (int i = 0; i < set.getNumSlots(); i++) { writer.writeBytes(set.maxLevels[i], set.minLevels[i]); - writer.skip(2); + writer.write(set.slotPadding[i] != null ? set.slotPadding[i] : new byte[NUM_WATER_SLOT_PADDING_BYTES]); writer.writeInt(set.species[i]); } } @@ -248,6 +270,9 @@ public void setRadarSpecies(int idx, int species) private static final int NUM_RADAR_ENCOUNTER_SLOTS = 4; private static final int NUM_DUAL_SLOT_ENCOUNTER_SLOTS = 2; private static final int NUM_DUAL_SLOT_GAMES = 5; + private static final int NUM_FORM_PROBABILITY_BYTES = 24; + private static final int NUM_UNKNOWN_BYTES = 0x2C; + private static final int NUM_WATER_SLOT_PADDING_BYTES = 2; private static final int PLAT_FIELD_ENCOUNTER_SET_IDX = 0; enum DualSlot { diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/evolutions/EvolutionData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/evolutions/EvolutionData.java index cd21aac..95c7820 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/evolutions/EvolutionData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/evolutions/EvolutionData.java @@ -11,6 +11,8 @@ public class EvolutionData extends ArrayList implements GenericFileData { + private int fileSize = FIXED_FILE_SIZE; + public EvolutionData(BytesDataContainer files) { super(); @@ -30,7 +32,11 @@ public void setData(BytesDataContainer files) MemBuf dataBuf = MemBuf.create(file); MemBuf.MemBufReader reader = dataBuf.reader(); - for (int i = 0; i < file.length / 6; i++) + fileSize = Math.max(file.length, FIXED_FILE_SIZE); + + // everything the file holds is read - dropping entries here would silently discard them on save + int numEntries = file.length / 6; + for (int i = 0; i < numEntries; i++) { add(new EvolutionEntry(reader.readShort(), reader.readShort(), reader.readShort())); } @@ -42,6 +48,11 @@ public BytesDataContainer save() MemBuf dataBuf = MemBuf.create(); MemBuf.MemBufWriter writer = dataBuf.writer(); + if (size() > MAX_NUM_ENTRIES) + { + throw new RuntimeException("An evolution file can hold at most " + MAX_NUM_ENTRIES + " entries. Provided: " + size()); + } + for(EvolutionEntry entry : this) { writer.writeShort((short) entry.getMethod()); @@ -51,6 +62,13 @@ public BytesDataContainer save() writer.writeShort((short) 0); + // the game reads a fixed-size record regardless of how many evolutions are actually populated, + // so the subfile must always be emitted at its full length + if (writer.getPosition() < fileSize) + { + writer.writeByteNumTimes((byte) 0, fileSize - writer.getPosition()); + } + return new BytesDataContainer(GameFiles.EVOLUTIONS, null, dataBuf.reader().getBuffer()); } @@ -106,4 +124,5 @@ public void setResultSpecies(int resultSpecies) } public static final int MAX_NUM_ENTRIES = 7; + public static final int FIXED_FILE_SIZE = MAX_NUM_ENTRIES * 6 + 2; } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/items/ItemData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/items/ItemData.java index f64c069..6d98d74 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/items/ItemData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/items/ItemData.java @@ -27,6 +27,7 @@ public class ItemData implements GenericFileData int fieldFunction; // u8 int battleFunction; // u8 int workType; // u8 + int unknownPad; // u8 - unused byte at 0x0D, preserved verbatim //sleep, poison, burn, freeze, paralyze, confuse, attract, guard spec boolean[] statusRecoveries; // bitfield in this u8 @@ -50,6 +51,9 @@ public class ItemData implements GenericFileData int[] friendshipChangeAmounts; + int unknownBitfield2Bits; // upper nibble of the byte at 0x14, preserved verbatim + byte[] trailingPad; // trailing bytes of the entry, preserved verbatim + public ItemData(BytesDataContainer files) { setData(files); @@ -84,7 +88,7 @@ public void setData(BytesDataContainer files) fieldFunction = reader.readUInt8(); battleFunction = reader.readUInt8(); workType = reader.readUInt8(); - reader.skip(1); + unknownPad = reader.readUInt8(); statusRecoveries = new boolean[NUM_STATUS_RECOVERIES]; int recovery = reader.readUInt8(); @@ -135,6 +139,8 @@ public void setData(BytesDataContainer files) int bitfield2 = reader.readUInt8(); evYieldToggles[NUM_EV_YIELDS - 1] = (bitfield2 & 1) == 1; + unknownBitfield2Bits = bitfield2 & 0xf0; + friendshipChangeToggles = new boolean[NUM_FRIENDSHIP_CHANGE_FIELDS]; for (int i = 0; i < NUM_FRIENDSHIP_CHANGE_FIELDS; i++) { @@ -147,14 +153,17 @@ public void setData(BytesDataContainer files) evYields[i] = reader.readByte(); // s8 } - hpRecoveryAmount = reader.readByte(); - ppRecoveryAmount = reader.readByte(); + hpRecoveryAmount = reader.readByte() & 0xFF; + ppRecoveryAmount = reader.readByte() & 0xFF; friendshipChangeAmounts = new int[NUM_FRIENDSHIP_CHANGE_FIELDS]; for (int i = 0; i < NUM_FRIENDSHIP_CHANGE_FIELDS; i++) { friendshipChangeAmounts[i] = reader.readByte(); } + + // getBuffer() returns everything between the read and write positions - i.e. the unread remainder + trailingPad = reader.getBuffer(); } @Override @@ -175,7 +184,7 @@ public BytesDataContainer save() writer.writeShort((short) composite); writer.writeBytes(fieldFunction, battleFunction, workType); - writer.skip(1); + writer.writeBytes(unknownPad); composite = 0; for (int i = 0; i < statusRecoveries.length; i++) @@ -194,11 +203,11 @@ public BytesDataContainer save() for (int i = 1; i < 4; i += 2) { - composite = (statBoosts[i] & 0xf) | ((statBoosts[i + 1] & 0x3) << 4); + composite = (statBoosts[i] & 0xf) | ((statBoosts[i + 1] & 0xf) << 4); writer.writeBytes(composite); } - composite = (statBoosts[5] & 0xf) | ((statBoosts[6] & 0xf) << 4); + composite = (statBoosts[5] & 0xf) | ((statBoosts[6] & 0x3) << 4); composite |= (ppUpEffects[0] ? 1 : 0) << 6; composite |= (ppUpEffects[1] ? 1 : 0) << 7; writer.writeBytes(composite); @@ -221,13 +230,21 @@ public BytesDataContainer save() { composite |= (friendshipChangeToggles[i] ? 1 : 0) << i + 1; } + composite |= unknownBitfield2Bits & 0xf0; writer.writeBytes(composite); writer.writeBytes(evYields); - writer.writeBytes(hpRecoveryAmount, ppRecoveryAmount); + writer.writeBytes(hpRecoveryAmount & 0xFF, ppRecoveryAmount & 0xFF); writer.writeBytes(friendshipChangeAmounts); - writer.writeByteNumTimes((byte) 0, 2); + if (trailingPad != null && trailingPad.length != 0) + { + writer.write(trailingPad); + } + else + { + writer.writeByteNumTimes((byte) 0, 2); + } return new BytesDataContainer(GameFiles.ITEMS, null, dataBuf.reader().getBuffer()); } @@ -417,6 +434,20 @@ public int[] getStatBoosts() public void setStatBoosts(int[] statBoosts) { + if (statBoosts == null || statBoosts.length != NUM_STAT_BOOSTS) + { + throw new IllegalArgumentException("Stat boosts array must contain exactly " + NUM_STAT_BOOSTS + " entries"); + } + + for (int i = 0; i < NUM_STAT_BOOSTS; i++) + { + int max = (i == NUM_STAT_BOOSTS - 1) ? 0x3 : 0xf; + if (statBoosts[i] < 0 || statBoosts[i] > max) + { + throw new IllegalArgumentException("Stat boost at index " + i + " must be between 0 and " + max + " (got " + statBoosts[i] + ")"); + } + } + this.statBoosts = statBoosts; } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/learnsets/LearnsetData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/learnsets/LearnsetData.java index e95bb9c..b2701e4 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/learnsets/LearnsetData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/learnsets/LearnsetData.java @@ -51,10 +51,22 @@ public void setData(BytesDataContainer files) // reader.setPosition(0); short combinedValue; - while ((combinedValue = reader.readShort()) != (short) 0xFFFF) + boolean terminated = false; + while (reader.getPosition() + 2 <= file.length) { + combinedValue = reader.readShort(); + if (combinedValue == (short) 0xFFFF) + { + terminated = true; + break; + } add(new LearnsetEntry(getMoveId(combinedValue), getLevelLearned(combinedValue))); } + + if (!terminated) + { + throw new RuntimeException("Level-up learnset is missing its 0xFFFF terminator (file is " + file.length + " bytes long)"); + } } @Override diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/learnsets/LearnsetParser.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/learnsets/LearnsetParser.java index b153a9c..aa97acc 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/learnsets/LearnsetParser.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/learnsets/LearnsetParser.java @@ -47,9 +47,17 @@ public List generateDataList(Map narcs, Map data = new ArrayList<>(); - for (byte[] subfile : learnsets.getFiles()) + List subfiles = learnsets.getFiles(); + for (int idx = 0; idx < subfiles.size(); idx++) { - data.add(new LearnsetData(new BytesDataContainer(GameFiles.LEVEL_UP_LEARNSETS, null, subfile))); + try + { + data.add(new LearnsetData(new BytesDataContainer(GameFiles.LEVEL_UP_LEARNSETS, null, subfiles.get(idx)))); + } + catch (RuntimeException e) + { + throw new RuntimeException("Failed to parse level-up learnset entry " + idx + ": " + e.getMessage(), e); + } } return data; diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java index 91cc7be..30db80f 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java @@ -40,6 +40,8 @@ public class PersonalData implements GenericFileData private int runChance; // u8 private int dexColor; // u8:7 + private byte[] padding; // 2 bytes of padding, preserved verbatim so a save round-trips + private int unknownEvYieldBits; // bits 12-15 of the ev yield halfword, preserved verbatim private boolean flip; // u8:1 private boolean[] tmCompatibility; // u8[16], each TM is a single bit @@ -106,6 +108,7 @@ public void setData(BytesDataContainer files) baseExp = reader.readUInt8(); int evYields = reader.readUInt16(); + unknownEvYieldBits = evYields & 0xF000; hpEvYield = getHpEv(evYields); atkEvYield = getAtkEv(evYields); defEvYield = getDefEv(evYields); @@ -131,7 +134,7 @@ public void setData(BytesDataContainer files) dexColor = colorFlip & 0x7F; flip = ((colorFlip & 0x80) >> 7) == 1; - reader.skip(2); // 2 bytes padding + padding = reader.readBytes(NUMBER_PADDING_BYTES); // 2 bytes padding, preserved verbatim byte[] tmLearnset = reader.readBytes(16); this.tmCompatibility = new boolean[NUMBER_TM_HM_BITS]; @@ -152,7 +155,7 @@ public BytesDataContainer save() writer.writeShort((short)uncommonItem); writer.writeShort((short)rareItem); writer.writeBytes(genderRatio,hatchMultiplier,baseHappiness,expRate,eggGroup1,eggGroup2,ability1,ability2,runChance,getCombinedColorFlip()); - writer.skip(2); + writer.write(padding != null ? padding : new byte[NUMBER_PADDING_BYTES]); int[] tmLearnsetData = new int[16]; for (int i = 0; i < NUMBER_TM_HM_BITS; i++) @@ -224,19 +227,20 @@ public BytesDataContainer save() private short getCombinedEvShort() { int val = 0; - val |= hpEvYield; - val |= (atkEvYield << 2); - val |= (defEvYield << 4); - val |= (speedEvYield << 6); - val |= (spAtkEvYield << 8); - val |= (spDefEvYield << 10); + val |= (hpEvYield & 0x03); + val |= ((atkEvYield & 0x03) << 2); + val |= ((defEvYield & 0x03) << 4); + val |= ((speedEvYield & 0x03) << 6); + val |= ((spAtkEvYield & 0x03) << 8); + val |= ((spDefEvYield & 0x03) << 10); + val |= (unknownEvYieldBits & 0xF000); return (short) val; } private int getCombinedColorFlip() { - return dexColor | (flip ? 0x80 : 0); + return (dexColor & 0x7F) | (flip ? 0x80 : 0); } public int getHp() @@ -366,8 +370,8 @@ public int getHpEvYield() public void setHpEvYield(int hpEvYield) { - if (hpEvYield >= 5) - throw new RuntimeException("Maximum HP EV yield value is 4. Provided: " + hpEvYield); + if (hpEvYield < 0 || hpEvYield > 3) + throw new RuntimeException("Maximum HP EV yield value is 3. Provided: " + hpEvYield); this.hpEvYield = hpEvYield; } @@ -378,8 +382,8 @@ public int getAtkEvYield() public void setAtkEvYield(int atkEvYield) { - if (atkEvYield >= 5) - throw new RuntimeException("Maximum Attack EV yield value is 4. Provided: " + atkEvYield); + if (atkEvYield < 0 || atkEvYield > 3) + throw new RuntimeException("Maximum Attack EV yield value is 3. Provided: " + atkEvYield); this.atkEvYield = atkEvYield; } @@ -390,8 +394,8 @@ public int getDefEvYield() public void setDefEvYield(int defEvYield) { - if (defEvYield >= 5) - throw new RuntimeException("Maximum Defense EV yield value is 4. Provided: " + defEvYield); + if (defEvYield < 0 || defEvYield > 3) + throw new RuntimeException("Maximum Defense EV yield value is 3. Provided: " + defEvYield); this.defEvYield = defEvYield; } @@ -402,8 +406,8 @@ public int getSpeedEvYield() public void setSpeedEvYield(int speedEvYield) { - if (speedEvYield >= 5) - throw new RuntimeException("Maximum Speed EV yield value is 4. Provided: " + speedEvYield); + if (speedEvYield < 0 || speedEvYield > 3) + throw new RuntimeException("Maximum Speed EV yield value is 3. Provided: " + speedEvYield); this.speedEvYield = speedEvYield; } @@ -414,8 +418,8 @@ public int getSpAtkEvYield() public void setSpAtkEvYield(int spAtkEvYield) { - if (spAtkEvYield >= 5) - throw new RuntimeException("Maximum Special Attack EV yield value is 4. Provided: " + spAtkEvYield); + if (spAtkEvYield < 0 || spAtkEvYield > 3) + throw new RuntimeException("Maximum Special Attack EV yield value is 3. Provided: " + spAtkEvYield); this.spAtkEvYield = spAtkEvYield; } @@ -426,8 +430,8 @@ public int getSpDefEvYield() public void setSpDefEvYield(int spDefEvYield) { - if (spDefEvYield >= 5) - throw new RuntimeException("Maximum Special Defense EV yield value is 4. Provided: " + spDefEvYield); + if (spDefEvYield < 0 || spDefEvYield > 3) + throw new RuntimeException("Maximum Special Defense EV yield value is 3. Provided: " + spDefEvYield); this.spDefEvYield = spDefEvYield; } @@ -570,8 +574,8 @@ public int getDexColor() public void setDexColor(int dexColor) { - if (dexColor >= 129) - throw new RuntimeException("Maximum dex color value is 128. Provided: " + dexColor); + if (dexColor < 0 || dexColor > 0x7F) + throw new RuntimeException("Maximum dex color value is 127. Provided: " + dexColor); this.dexColor = dexColor; } @@ -596,6 +600,7 @@ public void setTmCompatibility(boolean[] tmCompatibility) } private static final int NUMBER_TM_HM_BITS = 128; + private static final int NUMBER_PADDING_BYTES = 2; protected static final int NUMBER_TMS_HMS = 100; private static int getHpEv(int x) diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalParser.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalParser.java index 99041b6..0a85ae3 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalParser.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalParser.java @@ -38,10 +38,42 @@ public class PersonalParser implements GenericParser { - public static final int[] tmMoveIdNumbers = new int[PersonalData.NUMBER_TMS_HMS]; - public static final int[] tmMoveTypes = new int[PersonalData.NUMBER_TMS_HMS]; - private static boolean hasReadTmMoveIds = false; - private static boolean hasReadTmMoveTypes = false; + // this parser is a singleton, so none of this may be static - otherwise the TM/HM table read out of + // ROM A would be written into ROM B + private final int[] tmMoveIdNumbers = new int[PersonalData.NUMBER_TMS_HMS]; + private final int[] tmMoveTypes = new int[PersonalData.NUMBER_TMS_HMS]; + // the palette index exactly as it was read, so that indices this editor does not recognize survive a save + private final int[] tmMovePaletteIndices = new int[PersonalData.NUMBER_TMS_HMS]; + private boolean hasReadTmMoveIds = false; + private boolean hasReadTmMoveTypes = false; + + /** + * Gets the move ID assigned to each TM/HM in the currently loaded ROM + * @return an int[] + */ + public int[] getTmMoveIdNumbers() + { + return tmMoveIdNumbers; + } + + /** + * Gets the move ID assigned to the given TM/HM in the currently loaded ROM + * @param tmID an int + * @return an int + */ + public int getTmMoveIdNumber(int tmID) + { + return tmMoveIdNumbers[tmID]; + } + + /** + * Gets the type of the move assigned to each TM/HM in the currently loaded ROM + * @return an int[] + */ + public int[] getTmMoveTypes() + { + return tmMoveTypes; + } @Override public List generateDataList(Map narcs, Map codeBinaries) @@ -84,7 +116,8 @@ public List generateDataList(Map narcs, Map processDataList(List data, Map 5; // rock case 0x19D -> 2; // flying case 0x262 -> 6; // bug + // an unrecognized index is reported as Normal, but the raw index is kept by the caller so that + // it can be written straight back out rather than rewritten to Normal's index default -> 0; //todo figure out how to handle ???/fairy }; @@ -242,13 +282,18 @@ private static int typeToTmHmPaletteIndex(int type) case 5 -> 0x19C; // rock case 2 -> 0x19D; // flying case 6 -> 0x262; // bug - case 9 -> 0x191; // ???/fairy - default -> 0x192; + // NOTE: this mapping is deliberately NOT a bijection. Type 9 (???/fairy) has no TM/HM palette of + // its own, so it shares Psychic's index and therefore reads back as Psychic (14). The same is + // true of every unrecognized type, which shares Normal's index. Callers which need to preserve + // an index they did not change must write the original index back instead of round-tripping it + // through these two methods. + case 9 -> 0x191; // ???/fairy - shares Psychic's palette, reads back as Psychic + default -> 0x192; // shares Normal's palette, reads back as Normal //todo figure out how to handle ???/fairy }; } - public static void updateTmType(int tmID, int moveID, List moves) + public void updateTmType(int tmID, int moveID, List moves) { tmMoveIdNumbers[tmID] = moveID; tmMoveTypes[tmID] = moves.get(moveID).getType(); diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteData.java index 5368212..845200f 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteData.java @@ -5,8 +5,11 @@ import io.github.turtleisaac.nds4j.images.Palette; import io.github.turtleisaac.pokeditor.formats.GenericFileData; import io.github.turtleisaac.pokeditor.formats.BytesDataContainer; +import io.github.turtleisaac.pokeditor.gamedata.Game; import io.github.turtleisaac.pokeditor.gamedata.GameFiles; +import java.util.Objects; + public class PokemonSpriteData implements GenericFileData { public static final int BATTLE_SPRITE_HEIGHT = 80; @@ -40,11 +43,57 @@ public class PokemonSpriteData implements GenericFileData private int partyIconPaletteIndex = 0; + private final boolean scanFrontToBack; + + private boolean femaleBackOffsetEmpty; + private boolean maleBackOffsetEmpty; + private boolean femaleFrontOffsetEmpty; + private boolean maleFrontOffsetEmpty; + + private boolean paletteEmpty; + private boolean shinyPaletteEmpty; + + /** + * Creates a PokemonSpriteData for a game which scans its sprites front-to-back + * (Platinum, HeartGold and SoulSilver - the games this module supports) + * @param files a BytesDataContainer + */ public PokemonSpriteData(BytesDataContainer files) { + this(files, true); + } + + /** + * Creates a PokemonSpriteData, deriving the sprite scan direction from the given game + * @param files a BytesDataContainer + * @param game a Game + */ + public PokemonSpriteData(BytesDataContainer files, Game game) + { + this(files, scanDirectionFor(game)); + } + + private PokemonSpriteData(BytesDataContainer files, boolean scanFrontToBack) + { + this.scanFrontToBack = scanFrontToBack; setData(files); } + /** + * Gets whether the given game scans its sprite data front-to-back + * @param game a Game + * @return a boolean + */ + public static boolean scanDirectionFor(Game game) + { + Objects.requireNonNull(game, "A game must be provided in order to determine the sprite scan direction"); + return switch (game) { + case Platinum, HeartGold, SoulSilver -> true; + // Diamond and Pearl seed their scanned sprite decoding from the last halfword rather than the first + case Diamond, Pearl -> false; + }; + } + @Override public void setData(BytesDataContainer files) { @@ -62,25 +111,35 @@ public void setData(BytesDataContainer files) byte[] femaleFrontHeightOffsetFile = files.get(GameFiles.BATTLE_SPRITE_HEIGHT, BattleSpriteHeightOffsetsPattern.FEMALE_FRONT_Y); byte[] maleFrontHeightOffsetFile = files.get(GameFiles.BATTLE_SPRITE_HEIGHT, BattleSpriteHeightOffsetsPattern.MALE_FRONT_Y); - palette = new Palette(paletteFile, 4); - shinyPalette = new Palette(shinyPaletteFile, 4); + paletteEmpty = paletteFile.length == 0; + shinyPaletteEmpty = shinyPaletteFile.length == 0; + + if (!paletteEmpty) + palette = new Palette(paletteFile, 4); + if (!shinyPaletteEmpty) + shinyPalette = new Palette(shinyPaletteFile, 4); if (femaleBackFile.length != 0) - femaleBack = new IndexedImage(femaleBackFile, 0, 0, 1, 1, true); + femaleBack = new IndexedImage(femaleBackFile, 0, 0, 1, 1, scanFrontToBack); if (maleBackFile.length != 0) - maleBack = new IndexedImage(maleBackFile, 0, 0, 1, 1, true); + maleBack = new IndexedImage(maleBackFile, 0, 0, 1, 1, scanFrontToBack); if (femaleFrontFile.length != 0) - femaleFront = new IndexedImage(femaleFrontFile, 0, 0, 1, 1, true); + femaleFront = new IndexedImage(femaleFrontFile, 0, 0, 1, 1, scanFrontToBack); if (maleFrontFile.length != 0) - maleFront = new IndexedImage(maleFrontFile, 0, 0, 1, 1, true); + maleFront = new IndexedImage(maleFrontFile, 0, 0, 1, 1, scanFrontToBack); + + femaleBackOffsetEmpty = femaleBackHeightOffsetFile.length == 0; + maleBackOffsetEmpty = maleBackHeightOffsetFile.length == 0; + femaleFrontOffsetEmpty = femaleFrontHeightOffsetFile.length == 0; + maleFrontOffsetEmpty = maleFrontHeightOffsetFile.length == 0; - if (femaleBackHeightOffsetFile.length != 0) + if (!femaleBackOffsetEmpty) femaleBackOffset = -(femaleBackHeightOffsetFile[0] & 0xff); - if (maleBackHeightOffsetFile.length != 0) + if (!maleBackOffsetEmpty) maleBackOffset = -(maleBackHeightOffsetFile[0] & 0xff); - if (femaleFrontHeightOffsetFile.length != 0) + if (!femaleFrontOffsetEmpty) femaleFrontOffset = -(femaleFrontHeightOffsetFile[0] & 0xff); - if (maleFrontHeightOffsetFile.length != 0) + if (!maleFrontOffsetEmpty) maleFrontOffset = -(maleFrontHeightOffsetFile[0] & 0xff); MemBuf buffer = MemBuf.create(metadata); @@ -95,7 +154,7 @@ public void setData(BytesDataContainer files) shadowXOffset = reader.readByte(); //byte 87 shadowSize = reader.readUInt8(); //byte 88 - partyIcon = new IndexedImage(partyIconFile, 4, 0, 1, 1, true); + partyIcon = new IndexedImage(partyIconFile, 4, 0, 1, 1, scanFrontToBack); } @Override @@ -106,22 +165,15 @@ public BytesDataContainer save() container.insert(GameFiles.BATTLE_SPRITES, BattleSpriteNarcPattern.MALE_BACK, maleBack != null ? maleBack.save() : new byte[] {}); container.insert(GameFiles.BATTLE_SPRITES, BattleSpriteNarcPattern.FEMALE_FRONT, femaleFront != null ? femaleFront.save() : new byte[] {}); container.insert(GameFiles.BATTLE_SPRITES, BattleSpriteNarcPattern.MALE_FRONT, maleFront != null ? maleFront.save() : new byte[] {}); - container.insert(GameFiles.BATTLE_SPRITES, BattleSpriteNarcPattern.PALETTE, palette.save()); - container.insert(GameFiles.BATTLE_SPRITES, BattleSpriteNarcPattern.SHINY_PALETTE, shinyPalette.save()); + container.insert(GameFiles.BATTLE_SPRITES, BattleSpriteNarcPattern.PALETTE, palette != null ? palette.save() : new byte[] {}); + container.insert(GameFiles.BATTLE_SPRITES, BattleSpriteNarcPattern.SHINY_PALETTE, shinyPalette != null ? shinyPalette.save() : new byte[] {}); - container.insert(GameFiles.PARTY_ICONS, null, partyIcon.save()); + container.insert(GameFiles.PARTY_ICONS, null, partyIcon != null ? partyIcon.save() : new byte[] {}); - byte[] femaleBackHeightOffsetFile = new byte[] {(byte) Math.abs(femaleBackOffset)}; - container.insert(GameFiles.BATTLE_SPRITE_HEIGHT, BattleSpriteHeightOffsetsPattern.FEMALE_BACK_Y, femaleBackHeightOffsetFile); - - byte[] maleBackHeightOffsetFile = new byte[] {(byte) Math.abs(maleBackOffset)}; - container.insert(GameFiles.BATTLE_SPRITE_HEIGHT, BattleSpriteHeightOffsetsPattern.MALE_BACK_Y, maleBackHeightOffsetFile); - - byte[] femaleFrontHeightOffsetFile = new byte[] {(byte) Math.abs(femaleFrontOffset)}; - container.insert(GameFiles.BATTLE_SPRITE_HEIGHT, BattleSpriteHeightOffsetsPattern.FEMALE_FRONT_Y, femaleFrontHeightOffsetFile); - - byte[] maleFrontHeightOffsetFile = new byte[] {(byte) Math.abs(maleFrontOffset)}; - container.insert(GameFiles.BATTLE_SPRITE_HEIGHT, BattleSpriteHeightOffsetsPattern.MALE_FRONT_Y, maleFrontHeightOffsetFile); + container.insert(GameFiles.BATTLE_SPRITE_HEIGHT, BattleSpriteHeightOffsetsPattern.FEMALE_BACK_Y, heightOffsetFile(femaleBackOffset, femaleBackOffsetEmpty)); + container.insert(GameFiles.BATTLE_SPRITE_HEIGHT, BattleSpriteHeightOffsetsPattern.MALE_BACK_Y, heightOffsetFile(maleBackOffset, maleBackOffsetEmpty)); + container.insert(GameFiles.BATTLE_SPRITE_HEIGHT, BattleSpriteHeightOffsetsPattern.FEMALE_FRONT_Y, heightOffsetFile(femaleFrontOffset, femaleFrontOffsetEmpty)); + container.insert(GameFiles.BATTLE_SPRITE_HEIGHT, BattleSpriteHeightOffsetsPattern.MALE_FRONT_Y, heightOffsetFile(maleFrontOffset, maleFrontOffsetEmpty)); MemBuf buffer = MemBuf.create(); MemBuf.MemBufWriter writer = buffer.writer(); @@ -138,6 +190,22 @@ public BytesDataContainer save() return container; } + /** + * Produces the height offset subfile for the given offset. A zero-length entry in the ROM has to stay + * zero-length, and the stored byte is the exact inverse of what setData reads, which + * negates it. + * + * @param offset an int containing the height offset + * @param wasEmpty a boolean containing whether the entry this came from was zero-length + * @return a byte[] + */ + private static byte[] heightOffsetFile(int offset, boolean wasEmpty) + { + if (wasEmpty) + return new byte[0]; + return new byte[] {(byte) (-offset)}; + } + public Palette getPalette() { diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java index eb95bfd..b75ee4b 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java @@ -20,9 +20,14 @@ public class PokemonSpriteParser implements GenericParser { - private static final int MAX_PARTY_ICON_PALETTE_VALUE = 2; + public static final int MAX_PARTY_ICON_PALETTE_VALUE = 2; - private static List partyIconStartingFiles; + private static final int NUM_PARTY_ICON_STARTING_FILES = 7; + + // this parser is a singleton, so these must NOT be static - otherwise the files read out of ROM A + // would be written into ROM B + private List partyIconStartingFiles; + private int partyIconPaletteTableLength = -1; @Override public List generateDataList(Map narcs, Map codeBinaries) @@ -58,11 +63,6 @@ public List generateDataList(Map narcs, Map< } MainCodeFile arm9 = (MainCodeFile) codeBinaries.get(GameCodeBinaries.ARM9); - MemBuf.MemBufReader arm9Reader = arm9.getPhysicalAddressBuffer().reader(); - arm9Reader.setPosition(Tables.PARTY_ICON_PALETTE.getPointerOffset()); - int offset = arm9Reader.readInt(); - arm9Reader.setPosition(offset - arm9.getRamStartAddress()); - Narc sprites = narcs.get(GameFiles.BATTLE_SPRITES); Narc spriteHeights = narcs.get(GameFiles.BATTLE_SPRITE_HEIGHT); @@ -70,9 +70,30 @@ public List generateDataList(Map narcs, Map< Narc partyIcons = narcs.get(GameFiles.PARTY_ICONS); ArrayList data = new ArrayList<>(); + PokemonSpriteData.BattleSpriteNarcPattern[] spritesNarcPattern = PokemonSpriteData.BattleSpriteNarcPattern.values(); + PokemonSpriteData.BattleSpriteHeightOffsetsPattern[] spriteHeightOffsetsPattern = PokemonSpriteData.BattleSpriteHeightOffsetsPattern.values(); + + int numSpecies = sprites.getFiles().size() / spritesNarcPattern.length; + + // read the entire party icon palette index table up front so the shared arm9 buffer is only + // touched while its lock is held + int[] partyIconPaletteIndices; + arm9.lock(); + try { + MemBuf.MemBufReader arm9Reader = arm9.getPhysicalAddressBuffer().reader(); + arm9Reader.setPosition(Tables.PARTY_ICON_PALETTE.getPointerOffset()); + int offset = arm9Reader.readInt(); + arm9Reader.setPosition(offset - arm9.getRamStartAddress()); + partyIconPaletteIndices = arm9Reader.readBytesI(numSpecies); + } + finally { + arm9.unlock(); + } + partyIconPaletteTableLength = partyIconPaletteIndices.length; + Palette partyIconPalette = new Palette(partyIcons.getFile(0), 4); partyIconStartingFiles = new ArrayList<>(); - for (int i = 0; i < 7; i++) + for (int i = 0; i < NUM_PARTY_ICON_STARTING_FILES; i++) { partyIconStartingFiles.add(partyIcons.getFile(i)); } @@ -80,16 +101,14 @@ public List generateDataList(Map narcs, Map< MemBuf spriteMetadataBuffer = MemBuf.create(spriteMetadata.getFile(0)); MemBuf.MemBufReader spriteMetadataReader = spriteMetadataBuffer.reader(); - PokemonSpriteData.BattleSpriteNarcPattern[] spritesNarcPattern = PokemonSpriteData.BattleSpriteNarcPattern.values(); - PokemonSpriteData.BattleSpriteHeightOffsetsPattern[] spriteHeightOffsetsPattern = PokemonSpriteData.BattleSpriteHeightOffsetsPattern.values(); - for (int i = 0; i < sprites.getFiles().size() / spritesNarcPattern.length; i++) + for (int i = 0; i < numSpecies; i++) { BytesDataContainer container = new BytesDataContainer(); for (PokemonSpriteData.BattleSpriteNarcPattern entry : spritesNarcPattern) { container.insert(GameFiles.BATTLE_SPRITES, entry, sprites.getFile((i*spritesNarcPattern.length) + entry.getIndex())); } - container.insert(GameFiles.PARTY_ICONS, null, partyIcons.getFile(i + 7)); + container.insert(GameFiles.PARTY_ICONS, null, partyIcons.getFile(i + NUM_PARTY_ICON_STARTING_FILES)); container.insert(GameFiles.BATTLE_SPRITE_METADATA, null, spriteMetadataReader.readBytes(89)); for (PokemonSpriteData.BattleSpriteHeightOffsetsPattern entry : spriteHeightOffsetsPattern) { @@ -98,9 +117,9 @@ public List generateDataList(Map narcs, Map< PokemonSpriteData species = new PokemonSpriteData(container); species.getPartyIcon().setPalette(partyIconPalette); - int val = arm9Reader.readUInt8(); - if (val <= MAX_PARTY_ICON_PALETTE_VALUE) - species.setPartyIconPaletteIndex(val); + // the raw value is stored even when it is out of the range the editor knows about, otherwise + // simply loading and saving a ROM would rewrite the table + species.setPartyIconPaletteIndex(partyIconPaletteIndices[i]); data.add(species); } @@ -140,11 +159,15 @@ public Map processDataList(List data, Map processDataList(List data, Map commandID >= 0x16 && commandID <= 0x1D && commandID != 0x1B; - private static final IntPredicate isEndCommand = commandId -> commandId == 0x2 || commandId == 0x16 || commandId == 0x1B; - private static final IntPredicate isDoIfCommand = commandID -> commandID == 28 || commandID == 29 || commandID == 225; + private static final int GOTO_IF_TRAINER_DEFEATED = 225; + + // 225 (goto_if_trainer_defeated) is a relative branch, so its destination has to be registered as a label + private static final IntPredicate isCallCommand = commandID -> (commandID >= 0x16 && commandID <= 0x1D && commandID != 0x1B) || commandID == GOTO_IF_TRAINER_DEFEATED; + // 0x15 is endstd (yield to parent context), which terminates the current run of commands just like end/goto/return + private static final IntPredicate isEndCommand = commandId -> commandId == 0x2 || commandId == 0x15 || commandId == 0x16 || commandId == 0x1B; + // 225 is NOT a comparator command - per Scrcmd_Hg.txt it takes a single relative destination and no comparator byte + private static final IntPredicate isDoIfCommand = commandID -> commandID == 28 || commandID == 29; private static final IntPredicate isMovementCommand = commandID -> commandID == 0x5E; private static final IntPredicate isEndMovementCommand = commandID -> commandID == 0xFE; // private static final IntPredicate isOverworldObjectCommand @@ -37,19 +42,62 @@ public class FieldScriptData extends GenericScriptData comparators.put(5, "DIFFERENT"); } + /** + * Determines whether the given macro parameter name identifies a branch destination which has to be + * resolved to a label + * @param parameterName a String containing the name of a parameter as declared by its macro + * @return a boolean + */ + private static boolean isLabelParameterName(String parameterName) + { + return parameterName.contains("dest") || parameterName.contains("sub") || parameterName.equals("arg0"); + } + private List scripts; + private int fileIndex = -1; + public FieldScriptData(BytesDataContainer files) { super(files); } + public FieldScriptData(BytesDataContainer files, int fileIndex) + { + super(); + this.fileIndex = fileIndex; + setData(files); + } + public FieldScriptData() { super(); scripts = new ArrayList<>(); } + /** + * Gets the index of this script file within the field scripts narc, or -1 if it is not known + * @return an int + */ + public int getFileIndex() + { + return fileIndex; + } + + /** + * Sets the index of this script file within the field scripts narc (used purely for error reporting) + * @param fileIndex an int + */ + public void setFileIndex(int fileIndex) + { + this.fileIndex = fileIndex; + } + + private String describeFile() + { + return "script file " + (fileIndex >= 0 ? String.valueOf(fileIndex) : "?"); + } + @Override public void setData(BytesDataContainer files) { @@ -93,7 +141,7 @@ public void setData(BytesDataContainer files) for (int i = 0; i < labelOffsets.size(); i++) { if (!actionOffsets.contains(labelOffsets.get(i))) - readAtOffset(dataBuf, globalScriptOffsets, labelOffsets, actionOffsets, visitedOffsets, labelOffsets.get(i), labelMap, false); + readAtOffset(dataBuf, globalScriptOffsets, labelOffsets, actionOffsets, visitedOffsets, labelOffsets.get(i), labelMap, actionMap, false); } } while (lastSize != labelOffsets.size()); @@ -102,7 +150,7 @@ public void setData(BytesDataContainer files) for (int i = 0; i < labelOffsets.size(); i++) { if (!actionOffsets.contains(labelOffsets.get(i))) - readAtOffset(dataBuf, globalScriptOffsets, labelOffsets, actionOffsets, visitedOffsets, labelOffsets.get(i), labelMap, true); + readAtOffset(dataBuf, globalScriptOffsets, labelOffsets, actionOffsets, visitedOffsets, labelOffsets.get(i), labelMap, actionMap, true); else readActionAtOffset(dataBuf, actionOffsets, visitedOffsets, actionMap, labelOffsets.get(i)); } @@ -127,7 +175,7 @@ else if (component instanceof ActionLabel actionLabel) scripts.sort(Comparator.comparingInt(ScriptLabel::getScriptID)); if (scripts.size() != globalScriptOffsets.size()) - throw new RuntimeException(String.format("the expected number of scripts (%d) does not match the actual located amount (%d)", globalScriptOffsets.size(), globalScriptOffsets.size())); + throw new RuntimeException(String.format("%s: the expected number of scripts (%d) does not match the actual located amount (%d)", describeFile(), globalScriptOffsets.size(), scripts.size())); for (ScriptComponent component : this) { @@ -138,7 +186,7 @@ else if (component instanceof ActionLabel actionLabel) for (int i = 0; i < command.parameters.length; i++) { String paramName = command.commandMacro.getParameters()[i]; - if ((paramName.contains("dest") || paramName.contains("sub"))) + if (isLabelParameterName(paramName)) { command.parameters[i] = "label_" + labels.indexOf(labelMap.get((Integer) command.parameters[i])); } @@ -149,7 +197,7 @@ else if (isMovementCommand.test(command.commandMacro.getId())) for (int i = 0; i < command.parameters.length; i++) { String paramName = command.commandMacro.getParameters()[i]; - if ((paramName.contains("dest") || paramName.contains("sub"))) + if (isLabelParameterName(paramName)) { command.parameters[i] = "action_" + actions.indexOf(actionMap.get((Integer) command.parameters[i])); } @@ -176,7 +224,7 @@ else if (paramName.contains("overworld")) // } } - private void readAtOffset(MemBuf dataBuf, ArrayList globalScriptOffsets, ArrayList labelOffsets, ArrayList actionOffsets, ArrayList visitedOffsets, int offset, HashMap labelMap, boolean finalRun) + private void readAtOffset(MemBuf dataBuf, ArrayList globalScriptOffsets, ArrayList labelOffsets, ArrayList actionOffsets, ArrayList visitedOffsets, int offset, HashMap labelMap, HashMap actionMap, boolean finalRun) { MemBuf.MemBufReader reader = dataBuf.reader(); if (visitedOffsets.contains(offset)) { @@ -186,8 +234,10 @@ private void readAtOffset(MemBuf dataBuf, ArrayList globalScriptOffsets reader.setPosition(offset); int currentPosition; + int end = dataBuf.writer().getPosition(); - while (reader.getPosition() < dataBuf.writer().getPosition()) + // a command is at least a 2 byte id, so stop as soon as a whole id no longer fits + while (reader.getPosition() + 2 <= end) { currentPosition = reader.getPosition(); if (finalRun && !visitedOffsets.contains(currentPosition)) @@ -219,6 +269,7 @@ else if (labelOffsets.contains(currentPosition)) else if (actionOffsets.contains(currentPosition)) { ActionLabel actionLabel = new ActionLabel("action_" + Integer.toHexString(currentPosition)); + actionMap.putIfAbsent(currentPosition, actionLabel); add(actionLabel); } } @@ -230,13 +281,18 @@ else if (actionOffsets.contains(currentPosition)) // System.err.println(commandID); CommandMacro commandMacro = FieldScriptParser.nativeCommands.get(commandID); if (commandMacro == null) { - throw new RuntimeException("Invalid command ID: " + commandID); + throw new RuntimeException(describeFile() + ": Invalid command ID: " + commandID + " at offset 0x" + Integer.toHexString(currentPosition)); } ScriptCommand command = new ScriptCommand(commandMacro); command.name = commandMacro.getName(); - command.parameters = commandMacro.readParameters(reader); + try { + command.parameters = commandMacro.readParameters(reader); + } + catch (IllegalStateException e) { + throw new RuntimeException(String.format("%s: the command \"%s\" at offset 0x%s runs past the end of the file", describeFile(), commandMacro.getName(), Integer.toHexString(currentPosition)), e); + } // if (command.parameters != null) // { @@ -307,7 +363,11 @@ private void readActionAtOffset(MemBuf dataBuf, ArrayList actionOffsets reader.setPosition(offset); - while (reader.getPosition() < dataBuf.writer().getPosition()) + int end = dataBuf.writer().getPosition(); + boolean terminated = false; + + // each movement is a 2 byte id plus a 2 byte parameter, so stop as soon as a whole record no longer fits + while (reader.getPosition() + 4 <= end) { if (!visitedOffsets.contains(reader.getPosition())) { @@ -331,6 +391,7 @@ private void readActionAtOffset(MemBuf dataBuf, ArrayList actionOffsets if (isEndMovementCommand.test(commandID)) { + terminated = true; break; } @@ -344,6 +405,11 @@ private void readActionAtOffset(MemBuf dataBuf, ArrayList actionOffsets // // command.parameters = commandMacro.readParameters(reader); } + + if (!terminated) + { + throw new RuntimeException(String.format("%s: the action sequence at offset 0x%s runs off the end of the file without an end-movement command", describeFile(), Integer.toHexString(offset))); + } } private void findAndReplaceSequencesWithConvenienceCommands() @@ -454,7 +520,7 @@ else if (component instanceof ActionLabel actionLabel) idx++; } - return 0; + throw new RuntimeException(String.format("%s: the label \"%s\" could not be resolved to an offset - it is referenced by a branch but never defined", describeFile(), labelName)); }; for (ScriptComponent component : this) @@ -753,9 +819,11 @@ public String[] getParameterStrings() { if (parameters[i] instanceof Integer val) { - if (val >= 0x4000) + // ScriptFile.g4's NUMBER token has no sign, so a negative value has to be emitted in + // hexadecimal or it cannot be lexed back in + if (val >= 0x4000 || val < 0) { - parameterStrings[i] = "0x" + Integer.toHexString((int) parameters[i]); + parameterStrings[i] = "0x" + Integer.toHexString(val); } else { @@ -841,9 +909,20 @@ public ActionCommand(String name, int id, int parameter) public ActionCommand(int id, int parameter) { this.name = String.valueOf(id); + this.id = id; this.parameter = parameter; } + public int getId() + { + return id; + } + + public int getParameter() + { + return parameter; + } + @Override public String toString() { diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/FieldScriptParser.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/FieldScriptParser.java index bf6a8e5..19ca16f 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/FieldScriptParser.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/FieldScriptParser.java @@ -130,9 +130,10 @@ public List generateDataList(Map narcs, Map< Narc scripts = narcs.get(GameFiles.FIELD_SCRIPTS); ArrayList data = new ArrayList<>(); -// int i = 0; - for (byte[] subfile : scripts.getFiles()) + List subfiles = scripts.getFiles(); + for (int fileIndex = 0; fileIndex < subfiles.size(); fileIndex++) { + byte[] subfile = subfiles.get(fileIndex); // System.out.print(i); // if (i == 271) { // System.currentTimeMillis(); @@ -145,9 +146,8 @@ public List generateDataList(Map narcs, Map< else { // System.out.println(" (Normal)"); - data.add(new FieldScriptData(new BytesDataContainer(GameFiles.FIELD_SCRIPTS, null, subfile))); + data.add(new FieldScriptData(new BytesDataContainer(GameFiles.FIELD_SCRIPTS, null, subfile), fileIndex)); } -// i++; } return data; diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/LevelScriptData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/LevelScriptData.java index 620713a..7018bd5 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/LevelScriptData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/LevelScriptData.java @@ -6,8 +6,8 @@ import java.util.ArrayList; import java.util.Arrays; +import java.util.List; import java.util.Objects; -import java.util.TreeSet; import java.util.stream.Collectors; // this class only exists because AdAstra is awesome and wrote the only tool (until now) which could work with them @@ -17,11 +17,14 @@ */ public class LevelScriptData extends GenericScriptData { - private boolean hasPadding = true; + // NOTE: no initializer - instance initializers run after the superclass constructor, which is what + // parses the file, so an initializer here would overwrite whatever setData() worked out + private boolean hasPadding; public LevelScriptData() { super(); + hasPadding = true; } public LevelScriptData(BytesDataContainer files) @@ -37,7 +40,8 @@ public void setData(BytesDataContainer files) throw new RuntimeException("Script file not provided to editor"); } - MemBuf dataBuf = MemBuf.create(files.get(GameFiles.FIELD_SCRIPTS, null)); + byte[] file = files.get(GameFiles.FIELD_SCRIPTS, null); + MemBuf dataBuf = MemBuf.create(file); MemBuf.MemBufReader reader = dataBuf.reader(); ArrayList temp = new ArrayList<>(); @@ -85,6 +89,7 @@ public void setData(BytesDataContainer files) if (reader.readUInt16() == 0 && dataBuf.writer().getPosition() < SMALLEST_TRIGGER_SIZE) { //todo come back here // LSTrigger.customInfo("This level script does nothing.", "Interesting..."); + hasPadding = false; return; } } @@ -107,6 +112,34 @@ public void setData(BytesDataContainer files) } } } + + // whether this file carries trailing padding is a property of the file, not a constant + hasPadding = file.length != getUnpaddedLength(); + } + + /** + * Calculates the number of bytes save() emits before any trailing padding is applied + * @return the unpadded length in bytes of this level script + */ + private int getUnpaddedLength() + { + if (isEmpty()) + return 4; + + int mapScreenLoadCount = 0; + int variableCount = 0; + for (ScriptComponent component : this) + { + if (component instanceof VariableValueTrigger) + variableCount++; + else if (component instanceof MapScreenLoadTrigger) + mapScreenLoadCount++; + } + + int length = mapScreenLoadCount * 5; + if (variableCount != 0) + length += 6 + variableCount * 6; + return length + 2; } @Override @@ -115,8 +148,8 @@ public BytesDataContainer save() MemBuf dataBuf = MemBuf.create(); MemBuf.MemBufWriter writer = dataBuf.writer(); - TreeSet tsMapScreenLoad = new TreeSet<>(); - TreeSet tsVariable = new TreeSet<>(); + List tsMapScreenLoad = new ArrayList<>(); + List tsVariable = new ArrayList<>(); if (!isEmpty()) { @@ -133,7 +166,7 @@ public BytesDataContainer save() for (LevelScriptTrigger lstm : tsMapScreenLoad) { writer.writeByte((byte) lstm.getTriggerType()); - writer.writeUInt32((byte) lstm.getScriptTriggered()); + writer.writeUInt32(lstm.getScriptTriggered()); } if (!tsVariable.isEmpty()) { diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandDiscoverer.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandDiscoverer.java index b6ef08e..5466f22 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandDiscoverer.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandDiscoverer.java @@ -55,7 +55,7 @@ else if (child instanceof MacrosParser.Id_lineContext) macro.setId(child.accept(new CommandMacroVisitor<>() { @Override - protected Integer idLineAction(int idNumber) + protected Integer idLineAction(int idNumber, int dataType) { return idNumber; } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandMacro.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandMacro.java index fb4b2e1..21ac8ad 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandMacro.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandMacro.java @@ -46,10 +46,20 @@ public void write(MemBuf memBuf, CommandWriter.LabelOffsetObtainer offsetObtaine if (parameters.length != 0) { + int providedCount = parameterValues == null ? 0 : parameterValues.length; + if (providedCount != parameters.length) + { + throw new RuntimeException(String.format("The command \"%s\" expects %d parameter(s) %s but %d were provided", name, parameters.length, Arrays.toString(parameters), providedCount)); + } + int idx = 0; for (String parameter : parameters) { Object param = parameterValues[idx++]; - if (param instanceof Number number) + if (param == null) + { + throw new RuntimeException(String.format("The parameter \"%s\" of the command \"%s\" was not provided a value", parameter, name)); + } + else if (param instanceof Number number) parameterToValueMap.put(parameter, number); else if (param instanceof String str) { @@ -73,8 +83,16 @@ else if (param instanceof String str) throw new RuntimeException(String.format("An invalid parameter was provided (%s) in \"%s\"", str, this)); } } + else + { + throw new RuntimeException(String.format("The parameter \"%s\" of the command \"%s\" was provided a value of an unsupported type (%s): %s", parameter, name, param.getClass().getName(), param)); + } } } + else if (parameterValues != null && parameterValues.length != 0) + { + throw new RuntimeException(String.format("The command \"%s\" takes no parameters but %d were provided", name, parameterValues.length)); + } CommandWriter commandWriter = new CommandWriter(memBuf.writer(), offsetObtainer, parameterToValueMap); commandWriter.visitEntry(entryContext); diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandMacroVisitor.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandMacroVisitor.java index b3bb04a..5675404 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandMacroVisitor.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandMacroVisitor.java @@ -15,7 +15,7 @@ public abstract class CommandMacroVisitor extends MacrosBaseVisitor @Override public T visitId_line(MacrosParser.Id_lineContext ctx) { - boolean foundShort = false; + int idDataType = -1; boolean foundValue = false; int idNumber = -1; for (int i = 0; i < ctx.getChildCount(); i++) @@ -25,7 +25,7 @@ public T visitId_line(MacrosParser.Id_lineContext ctx) int type = ((TerminalNodeImpl) c).symbol.getType(); if (type == MacrosLexer.SHORT || type == MacrosLexer.WORD) { - foundShort = true; + idDataType = type; } else if (type == MacrosLexer.NUMBER) { foundValue = true; @@ -34,14 +34,14 @@ else if (type == MacrosLexer.NUMBER) { } } - if (foundShort && foundValue) { - return idLineAction(idNumber); + if (idDataType != -1 && foundValue) { + return idLineAction(idNumber, idDataType); } return null; } - protected T idLineAction(int idNumber) { + protected T idLineAction(int idNumber, int dataType) { return null; } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandReader.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandReader.java index 902ff1c..6772ac0 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandReader.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandReader.java @@ -120,7 +120,7 @@ public Integer visitTerminal(TerminalNode node) return (int) parameterToValueMap.get(terminalNode.getText().substring(1)); } } else if (terminalNode.symbol.getType() == MacrosLexer.NUMBER) { - return Integer.parseInt(terminalNode.getText()); + return Integer.decode(terminalNode.getText()); } else if (terminalNode.symbol.getType() == MacrosLexer.CURRENT_OFFSET) { return reader.getPosition() - 4; } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandWriter.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandWriter.java index 5bbbfbe..ba3487b 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandWriter.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandWriter.java @@ -35,9 +35,15 @@ public CommandWriter(MemBuf.MemBufWriter writer, LabelOffsetObtainer offsetObtai } @Override - protected Integer idLineAction(int idNumber) + protected Integer idLineAction(int idNumber, int dataType) { - writer.writeShort((short) idNumber); + // the macro declares the width of its own id line - AI script macros use .word, field script + // macros use .short - so it has to be honoured rather than assumed to be a short + switch (dataType) { + case MacrosLexer.SHORT -> writer.writeShort((short) idNumber); + case MacrosLexer.WORD -> writer.writeInt(idNumber); + default -> throw new IllegalStateException("Unexpected command id width: " + dataType); + } return null; } @@ -87,7 +93,7 @@ public Integer visitTerminal(TerminalNode node) return (int) parameterToValueMap.get(text); } } else if (terminalNode.symbol.getType() == MacrosLexer.NUMBER) { - return Integer.parseInt(terminalNode.getText()); + return Integer.decode(terminalNode.getText()); } else if (terminalNode.symbol.getType() == MacrosLexer.CURRENT_OFFSET) { return writer.getPosition(); } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/ScriptDataProducer.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/ScriptDataProducer.java index 0dd911f..d65025f 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/ScriptDataProducer.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/ScriptDataProducer.java @@ -263,8 +263,10 @@ private Object visitCommandParameterHelper(ScriptFileParser.ParameterContext ctx if (type == ScriptFileLexer.NUMBER) { + // negative word parameters are emitted as their unsigned hexadecimal representation, + // which does not fit in a signed parse if (text.contains("0x")) - return Integer.parseInt(text.substring(2), 16); + return Integer.parseUnsignedInt(text.substring(2), 16); else return Integer.parseInt(text); } @@ -273,7 +275,7 @@ else if (type == ScriptFileLexer.NAME) if (text.startsWith("0x")) { try { - return Integer.parseInt(text.substring(2), 16); + return Integer.parseUnsignedInt(text.substring(2), 16); } catch(NumberFormatException ignored) {} } return text; @@ -291,6 +293,66 @@ else if (child instanceof ScriptFileParser.LabelContext labelContext) @Override public Void visitAction_command(ScriptFileParser.Action_commandContext ctx) { + List parameters = new ArrayList<>(); + + for (ParseTree child : ctx.children) + { + if (child instanceof ScriptFileParser.Action_parametersContext actionParametersContext) + { + if (actionParametersContext.children == null) + continue; + + for (ParseTree parametersChild : actionParametersContext.children) + { + if (parametersChild instanceof ScriptFileParser.Action_parameterContext parameterContext) + parameters.add(parameterContext.getText().trim()); + } + } + } + + if (parameters.size() != 2) + { + scriptCompilationException.addSuppressed(new ScriptCompilationException(String.format("An action requires a movement and a duration, but %d parameter(s) were provided: \"%s\"", parameters.size(), ctx.getText().trim()))); + return null; + } + + String movement = parameters.get(0); + + Integer id = null; + String name = null; + for (Map.Entry entry : FieldScriptParser.movementNames.entrySet()) + { + if (entry.getValue().equalsIgnoreCase(movement)) + { + id = entry.getKey(); + name = entry.getValue(); + break; + } + } + + if (id == null) + { + // Movements_Hg.json does not name every opcode, so a movement may legitimately be written as a number + try { + id = Integer.decode(movement); + } + catch (NumberFormatException e) { + scriptCompilationException.addSuppressed(new ScriptCompilationException(String.format("\"%s\" is not a valid movement name or id", movement))); + return null; + } + } + + int parameter; + try { + parameter = Integer.decode(parameters.get(1)); + } + catch (NumberFormatException e) { + scriptCompilationException.addSuppressed(new ScriptCompilationException(String.format("\"%s\" is not a valid duration for the movement \"%s\"", parameters.get(1), movement))); + return null; + } + + data.add(name != null ? new FieldScriptData.ActionCommand(name, id, parameter) : new FieldScriptData.ActionCommand(id, parameter)); + return super.visitAction_command(ctx); } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/text/TextBankData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/text/TextBankData.java index 786ff8c..d33068f 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/text/TextBankData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/text/TextBankData.java @@ -42,12 +42,44 @@ public class TextBankData extends ArrayList implements Gen private int seed; + private int bankIndex = -1; + public TextBankData(BytesDataContainer files) { super(); setData(files); } + public TextBankData(BytesDataContainer files, int bankIndex) + { + super(); + this.bankIndex = bankIndex; + setData(files); + } + + /** + * Gets the index of this text bank within the text narc, or -1 if it is not known + * @return an int + */ + public int getBankIndex() + { + return bankIndex; + } + + /** + * Sets the index of this text bank within the text narc (used purely for error reporting) + * @param bankIndex an int + */ + public void setBankIndex(int bankIndex) + { + this.bankIndex = bankIndex; + } + + private String describe(int messageIdx) + { + return "text bank " + (bankIndex >= 0 ? String.valueOf(bankIndex) : "?") + ", message " + messageIdx; + } + @Override public void setData(BytesDataContainer files) { @@ -56,7 +88,8 @@ public void setData(BytesDataContainer files) throw new RuntimeException("Text file not provided to editor"); } - MemBuf dataBuf = MemBuf.create(files.get(GameFiles.TEXT, null)); + byte[] file = files.get(GameFiles.TEXT, null); + MemBuf dataBuf = MemBuf.create(file); MemBuf.MemBufReader reader = dataBuf.reader(); int numEntries = reader.readUInt16(); @@ -73,6 +106,19 @@ public void setData(BytesDataContainer files) key = ((seed * (i + 1) * 0x2fd) & 0xffff) | ((seed * (i + 1) * 0x2fd0000) & 0xffff0000); offsets[i] = reader.readInt() ^ key; sizes[i] = reader.readInt() ^ key; + + if (sizes[i] < 0) + { + throw new TextEncodingException(describe(i) + " declares a negative length (" + sizes[i] + ")"); + } + if (offsets[i] < 0 || offsets[i] > file.length) + { + throw new TextEncodingException(describe(i) + " declares an out of bounds offset (" + offsets[i] + ", file is " + file.length + " bytes)"); + } + if (offsets[i] + sizes[i] * 2L > file.length) + { + throw new TextEncodingException(describe(i) + " runs past the end of the file (offset " + offsets[i] + ", length " + sizes[i] + " halfwords, file is " + file.length + " bytes)"); + } } @@ -108,13 +154,26 @@ public void setData(BytesDataContainer files) else if (c == 0xfffe) { // if the character is 0xfffe, then it's a special VAR case + int[] binaryString = binaryStrings[messageIdx]; + + if (j + 2 >= binaryString.length) + { + throw new TextEncodingException(describe(messageIdx) + " contains a truncated VAR() - the variable id and argument count run past the end of the message"); + } + text.append("VAR("); StringBuilder args = new StringBuilder(); - args.append(binaryStrings[messageIdx][++j]); - int argNum = binaryStrings[messageIdx][++j]; + args.append(binaryString[++j]); + int argNum = binaryString[++j]; + + if (argNum < 0 || j + argNum >= binaryString.length) + { + throw new TextEncodingException(describe(messageIdx) + " contains a VAR() declaring " + argNum + " argument(s), which runs past the end of the message"); + } + for (int k = 0; k < argNum; k++) { args.append(", "); - args.append(binaryStrings[messageIdx][++j]); + args.append(binaryString[++j]); } args.append(")"); text.append(args); @@ -145,8 +204,9 @@ public BytesDataContainer save() ArrayList> binaryStrings = new ArrayList<>(); // encode text and write it - for (Message msg : this) + for (int messageIdx = 0; messageIdx < size(); messageIdx++) { + Message msg = get(messageIdx); String message = msg.text; tmpBinaryString = new ArrayList<>(); @@ -156,13 +216,29 @@ public BytesDataContainer save() String sub = message.substring(j, Math.min(j + 4, message.length())); if (message.charAt(j) == '\\') { + if (j + 1 >= message.length()) + { + throw new TextEncodingException(describe(messageIdx) + " ends with a lone '\\' escape character"); + } + switch (message.charAt(j + 1)) { case 'r' -> tmpBinaryString.add(0x25bc); case 'n' -> tmpBinaryString.add(0xe000); case 'f' -> tmpBinaryString.add(0x25bd); default -> { - tmpBinaryString.add(Integer.parseInt(message.substring(j + 2, j + 6), 16)); + if (j + 6 > message.length()) + { + throw new TextEncodingException(describe(messageIdx) + " contains a truncated escape sequence \"" + message.substring(j) + "\" - four hexadecimal digits are required"); + } + + String hex = message.substring(j + 2, j + 6); + try { + tmpBinaryString.add(Integer.parseInt(hex, 16)); + } + catch (NumberFormatException e) { + throw new TextEncodingException(describe(messageIdx) + " contains a malformed escape sequence \"" + message.substring(j, j + 6) + "\"", e); + } j += 4; } } @@ -172,38 +248,47 @@ else if (sub.equals("VAR(")) { int endOfVar = message.indexOf(')', j); if (endOfVar == -1) { - throw new RuntimeException("Could not find end of VAR()"); + throw new TextEncodingException(describe(messageIdx) + ": could not find end of VAR()"); } String[] args = message.substring(j + 4, endOfVar).split(","); tmpBinaryString.add(0xfffe); - tmpBinaryString.add(Integer.parseInt(args[0].trim())); - tmpBinaryString.add(args.length - 1); + try { + tmpBinaryString.add(Integer.parseInt(args[0].trim())); + tmpBinaryString.add(args.length - 1); - for (int x = 1; x < args.length; x++) - { - tmpBinaryString.add(Integer.parseInt(args[x].trim())); + for (int x = 1; x < args.length; x++) + { + tmpBinaryString.add(Integer.parseInt(args[x].trim())); + } + } + catch (NumberFormatException e) { + throw new TextEncodingException(describe(messageIdx) + " contains a VAR() with a non-numeric argument: \"" + message.substring(j, endOfVar + 1) + "\"", e); } j = endOfVar; } else { - int val = 0; - try { - val = characters.get("getInt").get(String.valueOf(message.charAt(j))).asInt(); - } - catch(NullPointerException e) { - e.printStackTrace(); + char c = message.charAt(j); + JsonNode node = characters.get("getInt").get(String.valueOf(c)); + if (node == null) + { + throw new TextEncodingException(String.format("%s contains the character '%c' (U+%04X) at index %d, which has no Pokemon Gen 4 character encoding", describe(messageIdx), c, (int) c, j)); } - tmpBinaryString.add(val); + tmpBinaryString.add(node.asInt()); } } if (msg.compressed) { - binaryStrings.add(compress(tmpBinaryString.toArray(Integer[]::new))); + try { + binaryStrings.add(compress(tmpBinaryString.toArray(Integer[]::new))); + } + catch (RuntimeException e) { + throw new TextEncodingException(describe(messageIdx) + " is marked as 9-bit compressed but " + e.getMessage(), e); + } } else { @@ -366,9 +451,12 @@ protected static ArrayList compress(Integer[] uncompressedString) throw for (int c : uncompressedString) { - if ( (c >> 9) == 1) + // only 9 bits are available per character, and all-ones (0x1ff) is reserved as the terminator + // by decompress(), so anything outside 0x000-0x1fe would silently overwrite the characters + // which follow it + if (c < 0 || c >= 0x1ff) { - throw new RuntimeException(String.format("%04x cannot be compressed", c)); + throw new RuntimeException(String.format("character %04x cannot be represented in the 9-bit compressed encoding", c)); } container |= c << bitshift; @@ -418,6 +506,23 @@ public List getStringList() return output; } + /** + * Thrown when a text bank cannot be decoded from or encoded to the Pokemon Gen 4 text format. + * The message names the offending bank and message so the caller can report it to the user. + */ + public static class TextEncodingException extends RuntimeException + { + public TextEncodingException(String message) + { + super(message); + } + + public TextEncodingException(String message, Throwable cause) + { + super(message, cause); + } + } + public static class Message { String text; boolean compressed; diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/text/TextBankParser.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/text/TextBankParser.java index df8a3de..2310778 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/text/TextBankParser.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/text/TextBankParser.java @@ -46,9 +46,10 @@ public List generateDataList(Map narcs, Map data = new ArrayList<>(); - for (byte[] subfile : personal.getFiles()) + List subfiles = personal.getFiles(); + for (int idx = 0; idx < subfiles.size(); idx++) { - data.add(new TextBankData(new BytesDataContainer(GameFiles.TEXT, null, subfile))); + data.add(new TextBankData(new BytesDataContainer(GameFiles.TEXT, null, subfiles.get(idx)), idx)); } return data; diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/trainers/TrainerData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/trainers/TrainerData.java index 6c96c4b..b4e0242 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/trainers/TrainerData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/trainers/TrainerData.java @@ -41,6 +41,8 @@ public class TrainerData implements GenericFileData int[] items; boolean[] ai; + int unknownFlagBits; + long unknownAiBits; int battleType2; short unknown1; short unknown2; @@ -82,6 +84,7 @@ public void setData(BytesDataContainer files) int flag = reader.readUInt8(); movesEnabled = (flag & 1) == 1; itemEnabled = ((flag >> 1) & 1) == 1; + unknownFlagBits = flag & ~0b11; trainerClass = reader.readUInt8(); battleType = reader.readUInt8(); @@ -101,6 +104,7 @@ public void setData(BytesDataContainer files) { ai[i] = ((aiComposite >> i) & 1) == 1; } + unknownAiBits = aiComposite & ~((1L << NUMBER_AI_FLAGS) - 1) & 0xffffffffL; battleType2 = reader.readUInt8(); unknown1 = reader.readUInt8(); @@ -121,7 +125,7 @@ public void setData(BytesDataContainer files) int combinedMon = reader.readUInt16(); entry.setSpecies(combinedMon & 0x3ff); - entry.setAltForm(combinedMon >> 10); + entry.setAltForm((combinedMon >> 10) & 0x3f); if (itemEnabled) { @@ -144,7 +148,19 @@ public void setData(BytesDataContainer files) public void setTeamFromSmogon(String text, BiFunction stringReplacementFunction) { - trainerPartyEntries = SmogonTeamImporter.importSmogonTeam(text, stringReplacementFunction); + ArrayList imported = SmogonTeamImporter.importSmogonTeam(text, stringReplacementFunction); + + if (imported.size() > MAX_NUMBER_TRAINER_MONS) + { + throw new SmogonTeamImporter.SmogonImportException(String.format("A trainer can have at most %d Pokemon, but the provided team contains %d", MAX_NUMBER_TRAINER_MONS, imported.size())); + } + + if (imported.size() < MIN_NUMBER_TRAINER_MONS) + { + throw new SmogonTeamImporter.SmogonImportException(String.format("A trainer must have at least %d Pokemon, but the provided team contains %d", MIN_NUMBER_TRAINER_MONS, imported.size())); + } + + trainerPartyEntries = imported; } @Override @@ -154,7 +170,7 @@ public BytesDataContainer save() MemBuf.MemBufWriter writer = dataBuf.writer(); // trdata - int compositeFlags = (movesEnabled ? 1 : 0) | (itemEnabled ? 0b10 : 0); + int compositeFlags = (movesEnabled ? 1 : 0) | (itemEnabled ? 0b10 : 0) | (unknownFlagBits & ~0b11); writer.writeBytes(compositeFlags, trainerClass, battleType, trainerPartyEntries.size()); for (int i = 0; i < NUMBER_TRAINER_ITEMS; i++) { @@ -166,6 +182,7 @@ public BytesDataContainer save() { compositeAiFlags |= ((ai[i] ? 1 : 0) << i); } + compositeAiFlags |= (int) unknownAiBits; writer.writeInt(compositeAiFlags); writer.writeBytes(battleType2, unknown1, unknown2, unknown3); @@ -179,7 +196,7 @@ public BytesDataContainer save() { writer.writeBytes(entry.getDifficultyValue(), entry.getAbility()); writer.writeShort((short) entry.getLevel()); - writer.writeShort((short)( ( (entry.getAltForm() & 0x7) << 10) | (entry.getSpecies() & 0x3ff) ) ); + writer.writeShort((short)( ( (entry.getAltForm() & 0x3f) << 10) | (entry.getSpecies() & 0x3ff) ) ); if (itemEnabled) { @@ -388,8 +405,8 @@ public String toSmogonString(BiFunction importSmogonTeam(String t { SmogonTeamImporter importer = new SmogonTeamImporter(stringReplacementFunction); - SmogonTeamLexer lexer = new SmogonTeamLexer(CharStreams.fromString(text)); + // the grammar requires each species entry to start on a fresh line and the paste to end with one + String normalized = text; + if (!normalized.startsWith("\n") && !normalized.startsWith("\r")) + normalized = "\n" + normalized; + if (!normalized.endsWith("\n")) + normalized = normalized + "\n"; + + SmogonTeamLexer lexer = new SmogonTeamLexer(CharStreams.fromString(normalized)); CommonTokenStream tokens = new CommonTokenStream(lexer); SmogonTeamParser parser = new SmogonTeamParser(tokens); - importer.visitTeam(parser.team()); + SmogonTeamParser.TeamContext team = parser.team(); + + int syntaxErrors = parser.getNumberOfSyntaxErrors(); + if (syntaxErrors != 0) + { + throw new SmogonImportException(String.format("The provided team could not be understood - %d syntax error(s) were found in it", syntaxErrors)); + } + + importer.visitTeam(team); + + if (importer.trainerPartyEntries.size() > TrainerData.MAX_NUMBER_TRAINER_MONS) + { + throw new SmogonImportException(String.format("A trainer can have at most %d Pokemon, but the provided team contains %d", TrainerData.MAX_NUMBER_TRAINER_MONS, importer.trainerPartyEntries.size())); + } return importer.trainerPartyEntries; } @@ -124,6 +144,34 @@ public Void visitEffortValues(SmogonTeamParser.EffortValuesContext ctx) return super.visitEffortValues(ctx); } + @Override + public Void visitIndividualValues(SmogonTeamParser.IndividualValuesContext ctx) + { + // the gen 4 trainer format has a single "difficulty" byte rather than per-stat IVs, and the exporter + // writes the same value out for every stat, so the first entry is the one which matters + for (ParseTree child : ctx.children) + { + if (child instanceof SmogonTeamParser.EffortValueEntryContext entryContext) + { + for (ParseTree entryChild : entryContext.children) + { + if (entryChild instanceof TerminalNodeImpl terminalNode && terminalNode.symbol.getType() == SmogonTeamLexer.NUMBER) + { + int iv = Integer.parseInt(terminalNode.getText()); + if (iv < 0 || iv > MAX_IV) + { + throw new SmogonImportException("An IV must be between 0 and " + MAX_IV + ". Provided: " + iv); + } + current.setDifficultyValue(iv * MAX_DIFFICULTY_VALUE / MAX_IV); + return null; + } + } + } + } + + return null; + } + @Override public Void visitNature(SmogonTeamParser.NatureContext ctx) { @@ -151,6 +199,27 @@ public Void visitMove(SmogonTeamParser.MoveContext ctx) return null; } + /** + * The largest value an individual value can hold + */ + public static final int MAX_IV = 31; + + /** + * The largest value a trainer party entry's difficulty byte can hold + */ + public static final int MAX_DIFFICULTY_VALUE = 255; + + /** + * Thrown when a pasted Smogon team cannot be turned into a trainer's party + */ + public static class SmogonImportException extends RuntimeException + { + public SmogonImportException(String message) + { + super(message); + } + } + public enum SmogonStringSources { SPECIES, diff --git a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Game.java b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Game.java index 8535f17..b68ea65 100755 --- a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Game.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Game.java @@ -24,7 +24,14 @@ public enum Game public final String[] sheetList; public final String[] editorList; - private Region region; + /** + * @deprecated the region of the ROM currently being worked with is not a property of the game, it is a + * property of the individual ROM. This only exists so that callers of the deprecated + * {@link #getRegion()} keep working, and will be removed alongside it - use the + * {@link BaseRomInfo} returned by {@link #parseBaseRom(String)} instead. + */ + @Deprecated + private static Region lastParsedRegion; Game(String[] sheetList, String[] editorList) { @@ -32,12 +39,29 @@ public enum Game this.editorList= editorList; } + /** + * Gets the region of the most recently parsed base ROM. + * + * @return a Region, or null if no base ROM has been parsed yet + * @deprecated the region belongs to the ROM, not to the Game constant, which is shared by + * every ROM opened in this process. Use the {@link BaseRomInfo} returned by + * {@link #parseBaseRom(String)} and pass its region around explicitly instead. + */ + @Deprecated public Region getRegion() { - return region; + return lastParsedRegion; } - public static Game parseBaseRom(String baseRomGameCode) + /** + * The game and region identified by a base ROM's game code + * + * @param game a Game + * @param region a Region + */ + public record BaseRomInfo(Game game, Region region) {} + + public static BaseRomInfo parseBaseRom(String baseRomGameCode) { Game game = switch (baseRomGameCode.substring(0, 3)) { case "ADA" -> Game.Diamond; @@ -48,8 +72,9 @@ public static Game parseBaseRom(String baseRomGameCode) default -> throw new RuntimeException("Invalid game"); }; - game.region = Region.getRegion(baseRomGameCode.charAt(3)); - return game; + Region region = Region.getRegion(baseRomGameCode.charAt(3)); + lastParsedRegion = region; + return new BaseRomInfo(game, region); } public enum Region @@ -63,7 +88,7 @@ public enum Region EUROPE, SPAIN; - static Region getRegion(char c) + public static Region getRegion(char c) { return switch (c) { diff --git a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/GameCodeBinaries.java b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/GameCodeBinaries.java index c1541c4..20b5edb 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/GameCodeBinaries.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/GameCodeBinaries.java @@ -23,6 +23,7 @@ public static void initialize(Game baseROM) case HeartGold, SoulSilver -> { BATTLE.id = 12; } + default -> throw new UnsupportedOperationException("Unsupported base ROM: " + baseROM); } } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/GameFiles.java b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/GameFiles.java index fcb0d39..d9cd01a 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/GameFiles.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/GameFiles.java @@ -100,6 +100,7 @@ public static void initialize(Game baseROM) FIELD_SCRIPTS.path = "a/0/1/2"; TRAINER_AI_SCRIPTS.path = "a/0/9/9"; } + default -> throw new UnsupportedOperationException("Unsupported base ROM: " + baseROM); } } } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Tables.java b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Tables.java index d3546f9..6105bfa 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Tables.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Tables.java @@ -1,5 +1,7 @@ package io.github.turtleisaac.pokeditor.gamedata; +import java.util.Objects; + public enum Tables { PARTY_ICON_PALETTE, @@ -9,21 +11,55 @@ public enum Tables TM_HM_MOVES ; + private static final int UNSET_POINTER_OFFSET = -1; + private GameCodeBinaries pointerLocation; - private int pointerOffset; + private int pointerOffset = UNSET_POINTER_OFFSET; public GameCodeBinaries getPointerLocation() { + if (pointerLocation == null) + { + throw new IllegalStateException("The code binary containing the " + name() + " table is not known for the base ROM which is currently loaded"); + } return pointerLocation; } public int getPointerOffset() { + if (pointerOffset == UNSET_POINTER_OFFSET) + { + throw new IllegalStateException("The offset of the " + name() + " table is not known for the base ROM which is currently loaded"); + } return pointerOffset; } + /** + * Initializes the table pointers for the given base ROM + * + * @param baseROM a Game + * @deprecated use {@link #initialize(Game, Game.Region)} - relying on + * {@link Game#getRegion()} means the region of a previously opened ROM can leak into this one + */ + @Deprecated public static void initialize(Game baseROM) { + initialize(baseROM, baseROM.getRegion()); + } + + public static void initialize(Game baseROM, Game.Region region) + { + Objects.requireNonNull(baseROM, "A base ROM must be provided in order to initialize the table pointers"); + Objects.requireNonNull(region, "The region of the base ROM must be provided in order to initialize the table pointers - it comes from Game.parseBaseRom()"); + + // every constant is reset first, otherwise switching between ROMs leaves the previous ROM's + // pointers in place for any table the new ROM does not assign + for (Tables table : values()) + { + table.pointerLocation = null; + table.pointerOffset = UNSET_POINTER_OFFSET; + } + switch (baseROM) { case Platinum -> { PARTY_ICON_PALETTE.pointerLocation = GameCodeBinaries.ARM9; @@ -31,7 +67,7 @@ public static void initialize(Game baseROM) TRAINER_CLASS_PRIZE_MONEY.pointerLocation = GameCodeBinaries.BATTLE; TM_HM_MOVES.pointerLocation = GameCodeBinaries.ARM9; ITEMS.pointerLocation = GameCodeBinaries.ARM9; - switch (baseROM.getRegion()) { + switch (region) { case USA -> { PARTY_ICON_PALETTE.pointerOffset = 0x079f80; TRAINER_CLASS_PRIZE_MONEY.pointerOffset = 0x816c; @@ -88,7 +124,7 @@ public static void initialize(Game baseROM) TRAINER_CLASS_GENDER.pointerLocation = GameCodeBinaries.ARM9; TM_HM_MOVES.pointerLocation = GameCodeBinaries.ARM9; ITEMS.pointerLocation = GameCodeBinaries.ARM9; - switch (baseROM.getRegion()) { + switch (region) { case USA -> { PARTY_ICON_PALETTE.pointerOffset = 0x074408; TRAINER_CLASS_GENDER.pointerOffset = 0x073600; @@ -126,7 +162,7 @@ public static void initialize(Game baseROM) TRAINER_CLASS_GENDER.pointerLocation = GameCodeBinaries.ARM9; TM_HM_MOVES.pointerLocation = GameCodeBinaries.ARM9; ITEMS.pointerLocation = GameCodeBinaries.ARM9; - switch (baseROM.getRegion()) { + switch (region) { case USA -> { PARTY_ICON_PALETTE.pointerOffset = 0x074408; TRAINER_CLASS_GENDER.pointerOffset = 0x073600; @@ -177,6 +213,7 @@ public static void initialize(Game baseROM) } } } + default -> throw new UnsupportedOperationException("Unsupported base ROM: " + baseROM); } } } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/TextFiles.java b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/TextFiles.java index f0bfb11..794b85f 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/TextFiles.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/TextFiles.java @@ -13,9 +13,19 @@ public enum TextFiles TRAINER_TEXT, TYPE_NAMES; + // 0 is a valid bank index, so an unknown bank has to be represented by something which isn't + static final int UNKNOWN_BANK = -1; + private int value; - public int getValue() {return value;} + public int getValue() + { + if (value == UNKNOWN_BANK) + { + throw new IllegalStateException("The " + name() + " text bank index is not known for the base ROM which is currently loaded"); + } + return value; + } public static void initialize(Game baseROM) { @@ -50,6 +60,7 @@ public static void initialize(Game baseROM) TRAINER_TEXT.value = HGSS_TRAINER_TEXT.value; TYPE_NAMES.value = HGSS_TYPE_NAMES.value; } + default -> throw new UnsupportedOperationException("Unsupported base ROM: " + baseROM); } } @@ -83,11 +94,11 @@ enum SpecificTextBanks PLAT_TRAINER_CLASS_NAMES(619), HGSS_TRAINER_CLASS_NAMES(730), - DP_TRAINER_TEXT(0), //TODO change + DP_TRAINER_TEXT(TextFiles.UNKNOWN_BANK), //TODO find the real bank index PLAT_TRAINER_TEXT(617), //TODO change HGSS_TRAINER_TEXT(728), - DP_TYPE_NAMES(0), //TODO change + DP_TYPE_NAMES(TextFiles.UNKNOWN_BANK), //TODO find the real bank index PLAT_TYPE_NAMES(624), HGSS_TYPE_NAMES(735); diff --git a/src/main/resources/data/characters.json b/src/main/resources/data/characters.json index 2abad1c..bdf2cf2 100755 --- a/src/main/resources/data/characters.json +++ b/src/main/resources/data/characters.json @@ -244,7 +244,7 @@ "242": "⊗", "243": "⊘", "244": "＝", - "245": "ｚ", + "245": "～", "246": "：", "247": "；", "248": "．", @@ -3099,6 +3099,7 @@ "ｘ": "221", "ｙ": "222", "ｚ": "223", + "～": "245", "！": "225", "？": "226", "、": "227", diff --git a/src/test/java/io/github/turtleisaac/pokeditor/formats/GenericParserTest.java b/src/test/java/io/github/turtleisaac/pokeditor/formats/GenericParserTest.java index 4190978..335f7c4 100644 --- a/src/test/java/io/github/turtleisaac/pokeditor/formats/GenericParserTest.java +++ b/src/test/java/io/github/turtleisaac/pokeditor/formats/GenericParserTest.java @@ -4,30 +4,66 @@ import io.github.turtleisaac.nds4j.NintendoDsRom; import io.github.turtleisaac.nds4j.binaries.CodeBinary; import io.github.turtleisaac.pokeditor.gamedata.*; +import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.*; import static org.assertj.core.api.Assertions.assertThat; abstract class GenericParserTest { + /** + * The directory the test ROMs live in. These are copyrighted files which cannot be committed, so the + * suite skips rather than fails when they are not present. + */ + static final String ROM_DIRECTORY_PROPERTY = "rom.dir"; + + static final String PLATINUM_ROM_PROPERTY = "rom.platinum"; + static final String HEARTGOLD_ROM_PROPERTY = "rom.heartgold"; + + static final String DEFAULT_PLATINUM_ROM = "Platinum.nds"; + static final String DEFAULT_HEARTGOLD_ROM = "HeartGold.nds"; + protected GenericParser parser; protected NintendoDsRom rom; protected abstract GenericParser createParser(); + /** + * The name of the ROM file this test needs. Override this to run against a different game. + * @return a String + */ + protected String romFileName() + { + return System.getProperty(PLATINUM_ROM_PROPERTY, DEFAULT_PLATINUM_ROM); + } + @BeforeEach protected void setup() { parser = createParser(); - rom = NintendoDsRom.fromFile("Platinum.nds"); - Game game = Game.parseBaseRom(rom.getGameCode()); - GameFiles.initialize(game); - TextFiles.initialize(game); - GameCodeBinaries.initialize(game); - Tables.initialize(game); + } + + /** + * Loads the ROM this test needs, skipping the test if it is not present rather than failing the build + */ + protected void loadRom() + { + String fileName = romFileName(); + Path path = Path.of(System.getProperty(ROM_DIRECTORY_PROPERTY, ".")).resolve(fileName); + Assumptions.assumeTrue(Files.exists(path), + () -> "Skipping - no ROM at " + path.toAbsolutePath() + ". Provide one with -D" + ROM_DIRECTORY_PROPERTY + "="); + + rom = NintendoDsRom.fromFile(path.toString()); + Game.BaseRomInfo baseRomInfo = Game.parseBaseRom(rom.getGameCode()); + GameFiles.initialize(baseRomInfo.game()); + TextFiles.initialize(baseRomInfo.game()); + GameCodeBinaries.initialize(baseRomInfo.game()); + Tables.initialize(baseRomInfo.game(), baseRomInfo.region()); } @Test @@ -38,6 +74,8 @@ void parserNotNull() { @Test void outputMatchesInput() { + loadRom(); + HashMap map = new HashMap<>(); for (GameFiles gameFile : parser.getRequirements()) { map.put(gameFile, new Narc(rom.getFileByName(gameFile.getPath()))); @@ -52,8 +90,18 @@ void outputMatchesInput() { for (GameFiles gameFile : parser.getRequirements()) { Narc originalNarc = map.get(gameFile); Narc outputNarc = output.get(gameFile); + + assertThat(outputNarc) + .as("no output narc was produced for %s", gameFile) + .isNotNull(); + + assertThat(outputNarc.getFiles().size()) + .as("the number of subfiles in the %s narc changed", gameFile) + .isEqualTo(originalNarc.getFiles().size()); + for (int idx = 0; idx < originalNarc.getFiles().size(); idx++) { assertThat(outputNarc.getFile(idx)) + .as("subfile %d of the %s narc", idx, gameFile) .isEqualTo(originalNarc.getFile(idx)); } } diff --git a/src/test/java/io/github/turtleisaac/pokeditor/formats/ParserTests.java b/src/test/java/io/github/turtleisaac/pokeditor/formats/ParserTests.java index bf861aa..0d4e81d 100644 --- a/src/test/java/io/github/turtleisaac/pokeditor/formats/ParserTests.java +++ b/src/test/java/io/github/turtleisaac/pokeditor/formats/ParserTests.java @@ -3,7 +3,6 @@ import com.google.inject.Key; import com.google.inject.TypeLiteral; import io.github.turtleisaac.nds4j.Narc; -import io.github.turtleisaac.nds4j.NintendoDsRom; import io.github.turtleisaac.nds4j.binaries.CodeBinary; import io.github.turtleisaac.pokeditor.formats.encounters.JohtoEncounterData; import io.github.turtleisaac.pokeditor.formats.encounters.SinnohEncounterData; @@ -12,18 +11,15 @@ import io.github.turtleisaac.pokeditor.formats.learnsets.LearnsetData; import io.github.turtleisaac.pokeditor.formats.moves.MoveData; import io.github.turtleisaac.pokeditor.formats.personal.PersonalData; +import io.github.turtleisaac.pokeditor.formats.pokemon_sprites.PokemonSpriteData; import io.github.turtleisaac.pokeditor.formats.scripts.GenericScriptData; -import io.github.turtleisaac.pokeditor.formats.scripts.LevelScriptData; -import io.github.turtleisaac.pokeditor.formats.scripts.FieldScriptData; import io.github.turtleisaac.pokeditor.formats.scripts.FieldScriptParser; import io.github.turtleisaac.pokeditor.formats.text.TextBankData; import io.github.turtleisaac.pokeditor.formats.trainers.TrainerData; import io.github.turtleisaac.pokeditor.gamedata.*; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; -import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -33,7 +29,8 @@ public class ParserTests { - public static class PersonalTests extends GenericParserTest + @Nested + class PersonalTests extends GenericParserTest { @Override protected GenericParser createParser() @@ -42,7 +39,8 @@ protected GenericParser createParser() } } - public static class LearnsetsTests extends GenericParserTest + @Nested + class LearnsetsTests extends GenericParserTest { @Override protected GenericParser createParser() @@ -51,7 +49,8 @@ protected GenericParser createParser() } } - public static class EvolutionsTests extends GenericParserTest + @Nested + class EvolutionsTests extends GenericParserTest { @Override protected GenericParser createParser() @@ -60,7 +59,8 @@ protected GenericParser createParser() } } - public static class TrainersTests extends GenericParserTest + @Nested + class TrainersTests extends GenericParserTest { @Override protected GenericParser createParser() @@ -69,7 +69,8 @@ protected GenericParser createParser() } } - public static class MovesTests extends GenericParserTest + @Nested + class MovesTests extends GenericParserTest { @Override protected GenericParser createParser() @@ -78,7 +79,8 @@ protected GenericParser createParser() } } - public static class SinnohEncountersTests extends GenericParserTest + @Nested + class SinnohEncountersTests extends GenericParserTest { @Override protected GenericParser createParser() @@ -87,7 +89,8 @@ protected GenericParser createParser() } } - public static class JohtoEncountersTests extends GenericParserTest + @Nested + class JohtoEncountersTests extends GenericParserTest { @Override protected GenericParser createParser() @@ -95,21 +98,15 @@ protected GenericParser createParser() return injector.getInstance(Key.get(new TypeLiteral<>() {})); } - @BeforeEach @Override - protected void setup() + protected String romFileName() { - parser = createParser(); - rom = NintendoDsRom.fromFile("HeartGold.nds"); - Game game = Game.parseBaseRom(rom.getGameCode()); - GameFiles.initialize(game); - TextFiles.initialize(game); - GameCodeBinaries.initialize(game); - Tables.initialize(game); + return System.getProperty(HEARTGOLD_ROM_PROPERTY, DEFAULT_HEARTGOLD_ROM); } } - public static class ItemsTests extends GenericParserTest + @Nested + class ItemsTests extends GenericParserTest { @Override protected GenericParser createParser() @@ -118,7 +115,8 @@ protected GenericParser createParser() } } - public static class TextBankTests extends GenericParserTest + @Nested + class TextBankTests extends GenericParserTest { @Override protected GenericParser createParser() @@ -127,6 +125,16 @@ protected GenericParser createParser() } } + @Nested + class PokemonSpriteTests extends GenericParserTest + { + @Override + protected GenericParser createParser() + { + return injector.getInstance(Key.get(new TypeLiteral<>() {})); + } + } + @Nested class FieldScriptsTests extends GenericParserTest { @@ -136,23 +144,18 @@ protected GenericParser createParser() return new FieldScriptParser(); } - @BeforeEach @Override - protected void setup() + protected String romFileName() { - parser = createParser(); - rom = NintendoDsRom.fromFile("HeartGold.nds"); - Game game = Game.parseBaseRom(rom.getGameCode()); - GameFiles.initialize(game); - TextFiles.initialize(game); - GameCodeBinaries.initialize(game); - Tables.initialize(game); + return System.getProperty(HEARTGOLD_ROM_PROPERTY, DEFAULT_HEARTGOLD_ROM); } @Test @Override void outputMatchesInput() { + loadRom(); + HashMap map = new HashMap<>(); for (GameFiles gameFile : parser.getRequirements()) { map.put(gameFile, new Narc(rom.getFileByName(gameFile.getPath()))); @@ -164,49 +167,30 @@ void outputMatchesInput() List data = parser.generateDataList(map, codeBinaries); Map output = parser.processDataList(data, codeBinaries); - int nonExactMatches = 0; - for (GameFiles gameFile : parser.getRequirements()) { Narc originalNarc = map.get(gameFile); Narc outputNarc = output.get(gameFile); + + assertThat(outputNarc) + .as("no output narc was produced for %s", gameFile) + .isNotNull(); + + assertThat(outputNarc.getFiles().size()) + .as("the number of script files changed") + .isEqualTo(originalNarc.getFiles().size()); + for (int idx = 0; idx < originalNarc.getFiles().size(); idx++) { byte[] outputFile = outputNarc.getFile(idx); - if (Arrays.equals(originalNarc.getFile(idx), outputFile)) - { - assertThat(outputFile) - .isEqualTo(originalNarc.getFile(idx)); - } - else if (outputFile.length != 0) - { - BytesDataContainer container = new BytesDataContainer(); - container.insert(GameFiles.FIELD_SCRIPTS, null, outputFile); - - GenericScriptData scriptData; - if (FieldScriptParser.testFileIsLevelScript(originalNarc.getFile(idx))) - scriptData = new LevelScriptData(container); - else - scriptData = new FieldScriptData(container); - - container = scriptData.save(); - byte[] rebuiltResult = container.get(GameFiles.FIELD_SCRIPTS, null); - - if (Arrays.equals(rebuiltResult, outputFile)) - { - System.out.println("Valid but non-1:1 Match: File " + idx); - nonExactMatches++; - } - else - { - System.err.println("File did not match original, attempted conditional rebuild but failed"); - } - - assertThat(rebuiltResult) - .isEqualTo(outputFile); - } + + assertThat(outputFile) + .as("script file %d serialized to nothing", idx) + .isNotEmpty(); + + assertThat(outputFile) + .as("script file %d", idx) + .isEqualTo(originalNarc.getFile(idx)); } } - - System.out.printf("In total, there were %d valid but non-1:1 matching rebuilt field script files (%d 1:1 matches).\n", nonExactMatches, data.size()-nonExactMatches); } } } diff --git a/src/test/java/io/github/turtleisaac/pokeditor/formats/TestsInjector.java b/src/test/java/io/github/turtleisaac/pokeditor/formats/TestsInjector.java index 55b9024..646731c 100644 --- a/src/test/java/io/github/turtleisaac/pokeditor/formats/TestsInjector.java +++ b/src/test/java/io/github/turtleisaac/pokeditor/formats/TestsInjector.java @@ -12,6 +12,8 @@ import io.github.turtleisaac.pokeditor.formats.moves.MoveParser; import io.github.turtleisaac.pokeditor.formats.personal.PersonalData; import io.github.turtleisaac.pokeditor.formats.personal.PersonalParser; +import io.github.turtleisaac.pokeditor.formats.pokemon_sprites.PokemonSpriteData; +import io.github.turtleisaac.pokeditor.formats.pokemon_sprites.PokemonSpriteParser; import io.github.turtleisaac.pokeditor.formats.text.TextBankData; import io.github.turtleisaac.pokeditor.formats.text.TextBankParser; import io.github.turtleisaac.pokeditor.formats.trainers.TrainerData; @@ -109,5 +111,15 @@ protected void configure() } } - public static final Injector injector = Guice.createInjector(new PersonalModule(), new LearnsetsModule(), new EvolutionsModule(), new TrainersModule(), new MovesModule(), new SinnohEncountersModule(), new JohtoEncountersModule(), new ItemsModule(), new TextBankModule()); + static class PokemonSpriteModule extends AbstractModule { + @Override + protected void configure() + { + bind(new TypeLiteral>() {}) + .to(PokemonSpriteParser.class) + .in(Scopes.SINGLETON); + } + } + + public static final Injector injector = Guice.createInjector(new PersonalModule(), new LearnsetsModule(), new EvolutionsModule(), new TrainersModule(), new MovesModule(), new SinnohEncountersModule(), new JohtoEncountersModule(), new ItemsModule(), new TextBankModule(), new PokemonSpriteModule()); } From 282718a7f94d949ed66c96ccbb868c7a6370a9a2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 08:00:43 +0000 Subject: [PATCH 02/13] Restore signed semantics for fields the format defines as s8 Nds4j's MemBufReader.readByte() now returns an unsigned value in 0..255, matching Buffer.readByte() and readUInt8(). Five fields here are defined by the Gen IV formats as signed 8-bit and were relying on the old signed behaviour, so they began parsing as large positive numbers instead: MoveData.priority -7 (Trick Room, Roar) read back as 249 ItemData.evYields EV-reducing berries yield negative EVs ItemData.friendshipChangeAmounts bitter berries lower friendship PokemonSpriteData.globalFrontYOffset / shadowXOffset signed offsets The bytes were never at risk: every write path narrows back through a byte, so 249 and -7 both serialize to 0xF9 and the file round-trips either way. What broke is the value. PokEditor declares the priority column's range as {-128, 127}, so the sheet would have displayed 249 and then rejected it as out of range on the next edit. Each read now converts explicitly at the point of the read. PokemonSpriteData .unknownByte is left unsigned: it is an opaque passthrough written back with a cast, so it has no signed interpretation to preserve. Checked and deliberately unchanged: - ItemData hp/ppRecoveryAmount are genuinely unsigned (0xFF is the "restore all" sentinel) and already mask. - LearnsetData reads its packed halfword as a signed short, but compares the terminator in the short domain and masks both extracted fields, so sign extension cannot reach either value. - EvolutionData's halfwords never exceed 0x7FFF in practice. Adds SignedFieldTest, which asserts both halves of the property: parsed values carry the right sign across the whole 0..255 domain, and the bytes still round-trip. It needs no ROM. Reverting the priority cast fails two of its tests while the round-trip test keeps passing, which is the distinction being drawn here. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob --- .../pokeditor/formats/items/ItemData.java | 4 +- .../pokeditor/formats/moves/MoveData.java | 2 +- .../pokemon_sprites/PokemonSpriteData.java | 4 +- .../pokeditor/formats/SignedFieldTest.java | 145 ++++++++++++++++++ 4 files changed, 150 insertions(+), 5 deletions(-) create mode 100644 src/test/java/io/github/turtleisaac/pokeditor/formats/SignedFieldTest.java diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/items/ItemData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/items/ItemData.java index 6d98d74..2a7db03 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/items/ItemData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/items/ItemData.java @@ -150,7 +150,7 @@ public void setData(BytesDataContainer files) evYields = new int[NUM_EV_YIELDS]; for (int i = 0; i < NUM_EV_YIELDS; i++) { - evYields[i] = reader.readByte(); // s8 + evYields[i] = (byte) reader.readByte(); // s8 } hpRecoveryAmount = reader.readByte() & 0xFF; @@ -159,7 +159,7 @@ public void setData(BytesDataContainer files) friendshipChangeAmounts = new int[NUM_FRIENDSHIP_CHANGE_FIELDS]; for (int i = 0; i < NUM_FRIENDSHIP_CHANGE_FIELDS; i++) { - friendshipChangeAmounts[i] = reader.readByte(); + friendshipChangeAmounts[i] = (byte) reader.readByte(); // s8: bitter berries lower friendship } // getBuffer() returns everything between the read and write positions - i.e. the unread remainder diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/moves/MoveData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/moves/MoveData.java index 424171c..7f25b4b 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/moves/MoveData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/moves/MoveData.java @@ -92,7 +92,7 @@ public void setData(BytesDataContainer files) target = reader.readUInt16(); - priority = reader.readByte(); + priority = (byte) reader.readByte(); // s8: negative for Trick Room, Roar, Whirlwind... flags = new boolean[NUM_MOVE_FLAGS]; int composite = reader.readUInt8(); diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteData.java index 845200f..520617b 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteData.java @@ -150,8 +150,8 @@ public void setData(BytesDataContainer files) unknownSection1 = reader.readBytes(42); //bytes 2-43 backMovement = reader.readUInt8(); //byte 44 unknownSection2 = reader.readBytes(41); //bytes 45-85 - globalFrontYOffset = reader.readByte(); //byte 86 - shadowXOffset = reader.readByte(); //byte 87 + globalFrontYOffset = (byte) reader.readByte(); //byte 86, s8 + shadowXOffset = (byte) reader.readByte(); //byte 87, s8 shadowSize = reader.readUInt8(); //byte 88 partyIcon = new IndexedImage(partyIconFile, 4, 0, 1, 1, scanFrontToBack); diff --git a/src/test/java/io/github/turtleisaac/pokeditor/formats/SignedFieldTest.java b/src/test/java/io/github/turtleisaac/pokeditor/formats/SignedFieldTest.java new file mode 100644 index 0000000..a89fdd0 --- /dev/null +++ b/src/test/java/io/github/turtleisaac/pokeditor/formats/SignedFieldTest.java @@ -0,0 +1,145 @@ +package io.github.turtleisaac.pokeditor.formats; + +import io.github.turtleisaac.pokeditor.formats.items.ItemData; +import io.github.turtleisaac.pokeditor.formats.moves.MoveData; +import io.github.turtleisaac.pokeditor.gamedata.GameFiles; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Sign handling for the fields the Gen IV formats define as signed 8-bit. + *

+ * The underlying buffer returns an unsigned byte, so any field that the format defines + * as signed has to be converted at the point of the read. Getting this wrong does not corrupt the + * file — every write path narrows back through a {@code byte}, so the bytes round-trip + * either way — but it corrupts the value: a move priority of -7 becomes 249, which + * is what the editor then displays and range-checks. + *

+ * These tests build records from raw bytes rather than from a ROM, so they run everywhere. Each + * asserts both halves of the property: the parsed value carries the right sign, and the bytes + * still survive a round trip. + */ +@DisplayName("Signed 8-bit fields keep their sign") +class SignedFieldTest +{ + /** Byte 0xF9 is -7 as a signed value and 249 as an unsigned one. */ + private static final int RAW = 0xF9; + private static final int SIGNED = -7; + + private static BytesDataContainer container(GameFiles file, byte[] data) + { + return new BytesDataContainer(file, null, data); + } + + private static byte[] moveRecordWithPriority(int rawPriority) + { + // The moves record is a fixed 16-byte struct; priority sits at offset 0x0A, after + // effect(2), category(1), power(1), type(1), accuracy(1), pp(1), effectChance(1), + // target(2). + byte[] record = new byte[16]; + record[0x0A] = (byte) rawPriority; + return record; + } + + @Test + @DisplayName("move priority parses negative, as Trick Room and Roar require") + void movePriorityIsSigned() + { + MoveData move = new MoveData(container(GameFiles.MOVES, moveRecordWithPriority(RAW))); + + assertThat(move.getPriority()) + .as("0x%02X in the priority field is %d, not %d", RAW, SIGNED, RAW) + .isEqualTo(SIGNED); + } + + @Test + @DisplayName("move priority survives a round trip regardless of sign") + void movePriorityRoundTrips() + { + for (int raw = 0; raw <= 0xFF; raw++) + { + byte[] original = moveRecordWithPriority(raw); + MoveData move = new MoveData(container(GameFiles.MOVES, original)); + byte[] written = move.save().get(GameFiles.MOVES, null); + + assertThat(written[0x0A]) + .as("priority byte 0x%02X must survive parse then save", raw) + .isEqualTo((byte) raw); + } + } + + @Test + @DisplayName("the full signed byte domain parses to the right value") + void movePriorityDomain() + { + for (int raw = 0; raw <= 0xFF; raw++) + { + MoveData move = new MoveData(container(GameFiles.MOVES, moveRecordWithPriority(raw))); + assertThat(move.getPriority()) + .as("raw byte 0x%02X", raw) + .isEqualTo((int) (byte) raw) + .isBetween(-128, 127); + } + } + + @Test + @DisplayName("item EV yields and friendship changes keep their sign across a save") + void itemSignedFieldsSurviveRoundTrip() + { + // Expressed without reference to field offsets: set the signed fields, save, reload, and + // require the values to come back as they went in. That is the property a user cares + // about, and it stays true if the record layout is ever revised. + ItemData item = new ItemData(container(GameFiles.ITEMS, new byte[ITEM_RECORD_SIZE])); + item.setEvYields(new int[]{SIGNED, SIGNED, SIGNED, SIGNED, SIGNED, SIGNED}); + item.setFriendshipChangeAmounts(new int[]{SIGNED, SIGNED, SIGNED}); + + ItemData reloaded = new ItemData( + container(GameFiles.ITEMS, item.save().get(GameFiles.ITEMS, null))); + + assertThat(reloaded.getEvYields()) + .as("EV-reducing berries yield negative EVs, so the sign has to survive") + .containsOnly(SIGNED); + assertThat(reloaded.getFriendshipChangeAmounts()) + .as("bitter berries lower friendship, so the sign has to survive") + .containsOnly(SIGNED); + } + + @Test + @DisplayName("every signed byte value survives an item save/reload") + void itemSignedDomain() + { + for (int raw = 0; raw <= 0xFF; raw++) + { + int signed = (byte) raw; + ItemData item = new ItemData(container(GameFiles.ITEMS, new byte[ITEM_RECORD_SIZE])); + item.setFriendshipChangeAmounts(new int[]{signed, signed, signed}); + + ItemData reloaded = new ItemData( + container(GameFiles.ITEMS, item.save().get(GameFiles.ITEMS, null))); + + assertThat(reloaded.getFriendshipChangeAmounts()) + .as("friendship change %d (raw 0x%02X)", signed, raw) + .containsOnly(signed); + } + } + + @Test + @DisplayName("an item record round trips byte-for-byte") + void itemRecordRoundTrips() + { + // Whatever the sign handling, the bytes themselves must be preserved exactly. + ItemData item = new ItemData(container(GameFiles.ITEMS, new byte[ITEM_RECORD_SIZE])); + byte[] first = item.save().get(GameFiles.ITEMS, null); + byte[] second = new ItemData(container(GameFiles.ITEMS, first)) + .save().get(GameFiles.ITEMS, null); + + assertThat(second) + .as("parse then save must be a fixed point") + .isEqualTo(first); + } + + /** The fixed portion of an items record is 34 bytes; anything beyond is trailing padding. */ + private static final int ITEM_RECORD_SIZE = 36; +} From ed0a57548037b81f415e21f43989f8cff057c6bc Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 08:21:33 +0000 Subject: [PATCH 03/13] Depend on Nds4j 1.0.0 Nds4j is released as 1.0.0 in the companion branch; 0.1.0 no longer exists, so this declaration has to move with it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 17f2dbc..9b7b9af 100644 --- a/pom.xml +++ b/pom.xml @@ -55,7 +55,7 @@ io.github.turtleisaac Nds4j - 0.1.0 + 1.0.0 From 94bf999326a9dff70c25d9c92eafba6b63abdc12 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 08:36:59 +0000 Subject: [PATCH 04/13] Release 1.0.0 Cut from 1.0-SNAPSHOT so that PokEditor 3.2.0 -- a release version -- no longer depends on a mutable snapshot, which would make that build irreproducible. The audit in this branch is where the API changed, so this is the right place to draw the line: parseBaseRom returns a BaseRomInfo record rather than mutating region state on the Game enum, Tables.initialize takes the region explicitly, PersonalParser's TM/HM tables moved from statics to instance state, and several setters now throw where they previously corrupted silently. From here, semantic versioning puts further breaking changes behind 2.0.0. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 9b7b9af..3038dbe 100644 --- a/pom.xml +++ b/pom.xml @@ -6,7 +6,7 @@ io.github.turtleisaac PokEditor-Core - 1.0-SNAPSHOT + 1.0.0 20 From 1cb8c7187c8f4f829f527a8004390e874951f0b1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 08:40:05 +0000 Subject: [PATCH 05/13] Update README for the API and test changes in this branch The README is the documentation for external consumers -- it explicitly invites people to use this library in their own Gen 4 projects -- and its opening usage example no longer compiled: parseBaseRom returns a BaseRomInfo record now, and Tables.initialize takes the region explicitly. Breaking a documented public API without updating the document that teaches it left anyone following that guide with code that does not build. The example is now a compiled transcription rather than an eyeballed edit. Also corrects the build instructions, which told readers a HeartGold and a Platinum ROM were required in the repo root. That is no longer true: the suite builds and passes without one, skipping the tests that need real game data. Both places now describe -Drom.dir instead. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob --- README.md | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index b4d0b49..6c40a74 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ Feel free to use this library for your own projects relating to the Gen 4 Pokém ## Dependencies [Nds4j](https://github.com/turtleisaac/Nds4j), my library for interacting with, reading, editing, and writing the general proprietary Nintendo DS file formats, is required. -If you go to build PokEditor-Core yourself, please place your own **legally obtained** Pokémon HeartGold ROM in the root directory of the repo and name it "HeartGold.nds", and a Platinum ROM named "Platinum.nds". These will be accessed as part of the unit tests. +PokEditor-Core builds without a ROM. Some tests exercise round-tripping against real game data and are skipped when none is available; to run them, pass `-Drom.dir=` pointing at your own **legally obtained** HeartGold and Platinum ROMs. ## Usage As this is a library, there is nothing for you to run in terms of a JAR file or Main method. You need to write all the code to make use of my library yourself. @@ -17,11 +17,16 @@ When you go to use PokEditor-Core, the first thing your program needs to do is d ```java // create a NintendoDsRom object using something like NintendoDsRom.fromFile() NintendoDsRom rom = NintendoDsRom.fromFile("path to rom"); -Game game = Game.parseBaseRom(rom.getGameCode()); + +// parseBaseRom returns the game together with its region, rather than storing the +// region on the Game enum constant where every ROM in the process would share it +Game.BaseRomInfo baseRomInfo = Game.parseBaseRom(rom.getGameCode()); +Game game = baseRomInfo.game(); + GameFiles.initialize(game); TextFiles.initialize(game); GameCodeBinaries.initialize(game); -Tables.initialize(game); +Tables.initialize(game, baseRomInfo.region()); ``` Once you have ran the four `initialize` methods, your code can make use of the library without much further pain. Here is an example which works with species personal data: @@ -51,7 +56,7 @@ rom.saveToFile("output rom path", false); ## Troubleshooting -If you are having trouble building PokEditor-Core, make sure you have a "HeartGold.nds" and "Platinum.nds" ROM in the root directory of your local repo, as these are needed for the unit tests. +PokEditor-Core builds and its test suite passes without any ROM present — the tests that need one are skipped rather than failed. To run those too, point the build at a directory holding your own **legally obtained** ROMs with `-Drom.dir=`; the file names default to `HeartGold.nds` and `Platinum.nds` and can be overridden with `-Drom.heartgold` and `-Drom.platinum`. Additionally, there are some sources which need to be generated by maven/antlr4 which don't always get generated and compiled for some reason, so try running `mvn generate-sources` in the terminal/command-line from within the repo root to get those to appear. You may need to mark the directory `src/main/generated-sources` as a generated sources root afterwards. From dc0d9258365605e76a89cb0751e12fcb7136399a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 08:51:35 +0000 Subject: [PATCH 06/13] Add CI Runs the full suite on every push and pull request. Nds4j is not on Maven Central at the version this module depends on, so CI builds it from source. It prefers a branch of the same name when one exists and falls back to main otherwise: a change spanning both repositories is developed on matching branches, and building this module against a stale Nds4j would either report a failure that does not exist or hide a real one. Both paths were exercised against the live repositories before committing. Verified by running the same command locally. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob --- .github/workflows/build.yml | 65 +++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 .github/workflows/build.yml diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml new file mode 100644 index 0000000..a95932b --- /dev/null +++ b/.github/workflows/build.yml @@ -0,0 +1,65 @@ +name: Build + +on: + push: + branches: ['**'] + pull_request: + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +jobs: + test: + name: Test + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v4 + + - name: Set up JDK + uses: actions/setup-java@v4 + with: + distribution: temurin + # This module compiles at source/target 20. + java-version: '21' + cache: maven + + # Nds4j is not on Maven Central at the version this module depends on, so it has to be + # built from source. Prefer a branch of the same name when one exists: a change that + # spans both repositories is developed on matching branches, and testing this module + # against a stale Nds4j would report a failure that does not exist (or hide a real one). + - name: Resolve Nds4j branch + id: nds4j + run: | + ref="${{ github.head_ref || github.ref_name }}" + if git ls-remote --exit-code --heads https://github.com/turtleisaac/Nds4j.git "$ref" >/dev/null 2>&1; then + echo "Building against matching Nds4j branch: $ref" + else + echo "No Nds4j branch named '$ref'; falling back to main" + ref=main + fi + echo "ref=$ref" >> "$GITHUB_OUTPUT" + + - name: Check out Nds4j + uses: actions/checkout@v4 + with: + repository: turtleisaac/Nds4j + ref: ${{ steps.nds4j.outputs.ref }} + path: .upstream/Nds4j + + - name: Install Nds4j + run: mvn -B -ntp -q install -DskipTests -f .upstream/Nds4j/pom.xml + + - name: Build and test + # Round-trip tests against real game data need a retail ROM, which cannot be committed. + # They skip here and are run locally with -Drom.dir. Everything else runs. + run: mvn -B -ntp verify + + - name: Upload test reports + if: failure() + uses: actions/upload-artifact@v4 + with: + name: surefire-reports + path: '**/target/surefire-reports/**' + if-no-files-found: ignore From 7c2671d6a78ab23d2323be77e53f03cabd5ed04c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 20:20:22 +0000 Subject: [PATCH 07/13] Refuse to write a value the file cannot hold Every format narrows on the way out - a (short) cast, writeBytes' i2b, or an explicit mask - so a value that did not fit was written back as a different one with nothing reported. A learnset move of 512 was stored as move 0, which reads back as "learns nothing"; a level of 200 was stored as 72. The only way to notice was to reopen the file and find the wrong data. FieldWidth checks a value against the field it is about to occupy and names the field, the value and the limit when it does not fit. It is applied on the write path rather than in the setters: some formats load through their setters, and a file already containing an unusual value must still open. What must not happen is writing a value back out as something other than what was set. Applied to learnsets (7-bit level, 9-bit move), moves (every field, including priority as a signed byte), personal (whose setters guarded only the upper bound, so setHp(-1) was stored as 255) and evolutions. Two further defects surfaced while testing this: - Move 511 at level 127 packs to 0xFFFF, which is the end-of-learnset marker. Both fields are individually in range, so only the packed value reveals the problem: saving that entry wrote a terminator, and it and every entry after it disappeared on reload. It is now refused with an explanation. An exhaustive sweep of all 65536 combinations confirms this is the only such hole. - EvolutionData read its fields with readShort(), which sign-extends. The sheet offers 0..65535 for these columns, so a species ID above 32767 was accepted, written, and came back negative - a range the read path could not reproduce. They are unsigned in the ROM and are now read as such. This also means evolutions load through their entry constructor rather than by direct field assignment, so validation there would have to wait for the read to be correct first. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob --- .../pokeditor/formats/FieldWidth.java | 74 +++++++ .../formats/evolutions/EvolutionData.java | 12 +- .../formats/learnsets/LearnsetData.java | 23 ++- .../pokeditor/formats/moves/MoveData.java | 16 +- .../formats/personal/PersonalData.java | 20 +- .../pokeditor/formats/FieldWidthTest.java | 183 ++++++++++++++++++ 6 files changed, 314 insertions(+), 14 deletions(-) create mode 100644 src/main/java/io/github/turtleisaac/pokeditor/formats/FieldWidth.java create mode 100644 src/test/java/io/github/turtleisaac/pokeditor/formats/FieldWidthTest.java diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/FieldWidth.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/FieldWidth.java new file mode 100644 index 0000000..c430019 --- /dev/null +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/FieldWidth.java @@ -0,0 +1,74 @@ +package io.github.turtleisaac.pokeditor.formats; + +/** + * Checks that a value fits the field it is about to be written into. + *

+ * Every format in this package writes its fields by narrowing: {@code (short) value}, + * {@code writeBytes(value)}, or an explicit mask such as {@code value & 0x1FF}. Narrowing + * silently discards the high bits, so a value the editor accepted could be written back as + * a completely different one - a learnset move of 512 was stored as move 0 (no move at all), + * and a level of 200 as level 72. Nothing reported it, and the only way to notice was to + * reopen the file and read the wrong value back. + *

+ * These methods turn that silent loss into a failure that names the field and the value. + * They are deliberately called on the write path rather than in the setters: the + * setters are also fed by the load path in some formats, and a file already containing an + * unusual value must still open. What must not happen is writing a value back out as + * something other than what was set. + */ +public final class FieldWidth +{ + private FieldWidth() {} + + /** + * @param value the value about to be written + * @param numBits the number of bits the field actually occupies + * @param fieldName the field's name, for the failure message + * @return {@code value}, when it fits + * @throws IllegalArgumentException when it does not + */ + public static int bits(int value, int numBits, String fieldName) + { + int max = (1 << numBits) - 1; + if (value < 0 || value > max) + { + throw new IllegalArgumentException(String.format( + "%s is %d, which does not fit in the %d bits the file gives it (allowed: 0 to %d). " + + "Writing it would silently store a different value.", + fieldName, value, numBits, max)); + } + return value; + } + + /** + * An unsigned byte field, written through {@code writeBytes}. + */ + public static int u8(int value, String fieldName) + { + return bits(value, 8, fieldName); + } + + /** + * An unsigned 16-bit field, written through {@code writeShort}. + */ + public static int u16(int value, String fieldName) + { + return bits(value, 16, fieldName); + } + + /** + * A signed byte field. Distinct from {@link #u8} because the range is -128 to 127, not + * 0 to 255 - move priority is the field this exists for. + */ + public static int s8(int value, String fieldName) + { + if (value < Byte.MIN_VALUE || value > Byte.MAX_VALUE) + { + throw new IllegalArgumentException(String.format( + "%s is %d, which does not fit in the signed byte the file gives it " + + "(allowed: %d to %d). Writing it would silently store a different value.", + fieldName, value, Byte.MIN_VALUE, Byte.MAX_VALUE)); + } + return value; + } +} diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/evolutions/EvolutionData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/evolutions/EvolutionData.java index 95c7820..19ca711 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/evolutions/EvolutionData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/evolutions/EvolutionData.java @@ -2,6 +2,7 @@ import io.github.turtleisaac.nds4j.framework.MemBuf; import io.github.turtleisaac.pokeditor.formats.BytesDataContainer; +import io.github.turtleisaac.pokeditor.formats.FieldWidth; import io.github.turtleisaac.pokeditor.gamedata.GameFiles; import io.github.turtleisaac.pokeditor.formats.GenericFileData; @@ -38,7 +39,10 @@ public void setData(BytesDataContainer files) int numEntries = file.length / 6; for (int i = 0; i < numEntries; i++) { - add(new EvolutionEntry(reader.readShort(), reader.readShort(), reader.readShort())); + // unsigned: these are species and item IDs, which run past 0x7FFF in expanded ROMs. + // readShort() sign extended them, so a species above 32767 came back negative and the + // sheet's declared 0..65535 range was a value the read path could not reproduce. + add(new EvolutionEntry(reader.readUInt16(), reader.readUInt16(), reader.readUInt16())); } } @@ -55,9 +59,9 @@ public BytesDataContainer save() for(EvolutionEntry entry : this) { - writer.writeShort((short) entry.getMethod()); - writer.writeShort((short) entry.getRequirement()); - writer.writeShort((short) entry.getResultSpecies()); + writer.writeShort((short) FieldWidth.u16(entry.getMethod(), "Evolution method")); + writer.writeShort((short) FieldWidth.u16(entry.getRequirement(), "Evolution requirement")); + writer.writeShort((short) FieldWidth.u16(entry.getResultSpecies(), "Evolution result species")); } writer.writeShort((short) 0); diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/learnsets/LearnsetData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/learnsets/LearnsetData.java index b2701e4..68f0801 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/learnsets/LearnsetData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/learnsets/LearnsetData.java @@ -2,6 +2,7 @@ import io.github.turtleisaac.nds4j.framework.MemBuf; import io.github.turtleisaac.pokeditor.formats.BytesDataContainer; +import io.github.turtleisaac.pokeditor.formats.FieldWidth; import io.github.turtleisaac.pokeditor.gamedata.GameFiles; import io.github.turtleisaac.pokeditor.formats.GenericFileData; @@ -105,7 +106,24 @@ private static int getLevelLearned(short x) private static int produceLearnData(LearnsetEntry entry) { - return ((entry.getLevel() & 0x7f) << 9) | (entry.getMoveID() & 0x1ff); + // the two fields share one 16 bit word: 7 bits of level, 9 of move. masking instead of + // checking is what turned move 512 into move 0 and level 200 into level 72, silently. + int level = FieldWidth.bits(entry.getLevel(), 7, "Level"); + int moveID = FieldWidth.bits(entry.getMoveID(), 9, "Move ID"); + int packed = (level << 9) | moveID; + + // both fields are individually in range here, but this one combination packs to the + // same 0xFFFF the file uses to mark the end of the list. saving it writes a + // terminator, so the entry - and every entry after it - vanishes on reload. + if (packed == TERMINATOR) + { + throw new IllegalArgumentException(String.format( + "Move %d at level %d cannot be stored: the two together are indistinguishable " + + "from the end-of-learnset marker, so the entry would disappear when the " + + "file is read back. Use a different move or level.", + moveID, level)); + } + return packed; } public void sortLearnset() @@ -166,4 +184,7 @@ public void setLevel(int level) } public static final int MAX_NUM_ENTRIES = 20; + + /** the 16 bit value marking the end of the entry list; no real entry may pack to it */ + private static final int TERMINATOR = 0xFFFF; } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/moves/MoveData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/moves/MoveData.java index 7f25b4b..987fed1 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/moves/MoveData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/moves/MoveData.java @@ -21,6 +21,7 @@ import io.github.turtleisaac.nds4j.framework.MemBuf; import io.github.turtleisaac.pokeditor.formats.BytesDataContainer; +import io.github.turtleisaac.pokeditor.formats.FieldWidth; import io.github.turtleisaac.pokeditor.formats.learnsets.LearnsetData; import io.github.turtleisaac.pokeditor.gamedata.GameFiles; import io.github.turtleisaac.pokeditor.formats.GenericFileData; @@ -111,19 +112,24 @@ public BytesDataContainer save() MemBuf dataBuf = MemBuf.create(); MemBuf.MemBufWriter writer = dataBuf.writer(); - writer.writeShort((short) effect); - writer.writeBytes(category, power, type, accuracy, pp, effectChance); + // every one of these is narrowed on the way out - a short cast or writeBytes' i2b - + // so a value that does not fit is stored as a different value with nothing reported + writer.writeShort((short) FieldWidth.u16(effect, "Effect")); + writer.writeBytes(FieldWidth.u8(category, "Category"), FieldWidth.u8(power, "Power"), + FieldWidth.u8(type, "Type"), FieldWidth.u8(accuracy, "Accuracy"), + FieldWidth.u8(pp, "PP"), FieldWidth.u8(effectChance, "Effect chance")); - writer.writeShort((short) target); + writer.writeShort((short) FieldWidth.u16(target, "Target")); - writer.writeBytes(priority); + writer.writeBytes(FieldWidth.s8(priority, "Priority")); int composite = 0; for (int i = 0; i < NUM_MOVE_FLAGS; i++) { composite |= ((flags[i] ? 1 : 0) << i); } - writer.writeBytes(composite, contestEffect, contestType, 0, 0); + writer.writeBytes(composite, FieldWidth.u8(contestEffect, "Contest effect"), + FieldWidth.u8(contestType, "Contest type"), 0, 0); return new BytesDataContainer(GameFiles.MOVES, null, dataBuf.reader().getBuffer()); } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java index 30db80f..9772f58 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java @@ -2,6 +2,7 @@ import io.github.turtleisaac.nds4j.framework.MemBuf; import io.github.turtleisaac.pokeditor.formats.BytesDataContainer; +import io.github.turtleisaac.pokeditor.formats.FieldWidth; import io.github.turtleisaac.pokeditor.gamedata.GameFiles; import io.github.turtleisaac.pokeditor.formats.GenericFileData; @@ -150,11 +151,22 @@ public BytesDataContainer save() MemBuf dataBuf = MemBuf.create(); MemBuf.MemBufWriter writer = dataBuf.writer(); - writer.writeBytes(hp, atk, def, speed, spAtk, spDef, type1, type2, catchRate, baseExp); + // the setters guard only the upper bound, so a negative reached this point and was + // narrowed by writeBytes into a large positive one - setHp(-1) was stored as 255 + writer.writeBytes(FieldWidth.u8(hp, "HP"), FieldWidth.u8(atk, "Attack"), + FieldWidth.u8(def, "Defense"), FieldWidth.u8(speed, "Speed"), + FieldWidth.u8(spAtk, "Sp. Attack"), FieldWidth.u8(spDef, "Sp. Defense"), + FieldWidth.u8(type1, "Type 1"), FieldWidth.u8(type2, "Type 2"), + FieldWidth.u8(catchRate, "Catch rate"), FieldWidth.u8(baseExp, "Base experience")); writer.writeShort(getCombinedEvShort()); - writer.writeShort((short)uncommonItem); - writer.writeShort((short)rareItem); - writer.writeBytes(genderRatio,hatchMultiplier,baseHappiness,expRate,eggGroup1,eggGroup2,ability1,ability2,runChance,getCombinedColorFlip()); + writer.writeShort((short) FieldWidth.u16(uncommonItem, "Uncommon held item")); + writer.writeShort((short) FieldWidth.u16(rareItem, "Rare held item")); + writer.writeBytes(FieldWidth.u8(genderRatio, "Gender ratio"), + FieldWidth.u8(hatchMultiplier, "Hatch multiplier"), + FieldWidth.u8(baseHappiness, "Base happiness"), FieldWidth.u8(expRate, "Exp rate"), + FieldWidth.u8(eggGroup1, "Egg group 1"), FieldWidth.u8(eggGroup2, "Egg group 2"), + FieldWidth.u8(ability1, "Ability 1"), FieldWidth.u8(ability2, "Ability 2"), + FieldWidth.u8(runChance, "Run chance"), getCombinedColorFlip()); writer.write(padding != null ? padding : new byte[NUMBER_PADDING_BYTES]); int[] tmLearnsetData = new int[16]; diff --git a/src/test/java/io/github/turtleisaac/pokeditor/formats/FieldWidthTest.java b/src/test/java/io/github/turtleisaac/pokeditor/formats/FieldWidthTest.java new file mode 100644 index 0000000..2794741 --- /dev/null +++ b/src/test/java/io/github/turtleisaac/pokeditor/formats/FieldWidthTest.java @@ -0,0 +1,183 @@ +package io.github.turtleisaac.pokeditor.formats; + +import io.github.turtleisaac.pokeditor.formats.learnsets.LearnsetData; +import io.github.turtleisaac.pokeditor.gamedata.GameFiles; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * A field that does not fit its storage must fail loudly rather than be narrowed into a + * different value. + *

+ * The bounds asserted here are derived from the file layout, not from what the code + * currently does: a level occupies 7 bits and a move ID 9 bits of one shared 16-bit word, + * so the representable values are exactly 0..127 and 0..511. Anything outside that cannot + * be stored, and storing it anyway is data loss - move 512 became move 0, which reads back + * as "this Pokemon learns nothing at this level". + */ +class FieldWidthTest +{ + @Nested + @DisplayName("the width check itself") + class Widths + { + @Test + void everyValueThatFitsIsReturnedUnchanged() + { + // the check is the identity on its whole domain - it must not clamp, wrap or + // otherwise alter a value that was always fine + for (int i = 0; i <= 0xFF; i++) + assertThat(FieldWidth.u8(i, "f")).isEqualTo(i); + for (int i = 0; i <= 0xFFFF; i++) + assertThat(FieldWidth.u16(i, "f")).isEqualTo(i); + for (int i = Byte.MIN_VALUE; i <= Byte.MAX_VALUE; i++) + assertThat(FieldWidth.s8(i, "f")).isEqualTo(i); + } + + @Test + void theBoundariesThemselvesFit() + { + // an off-by-one in a bounds check shows up first at the bounds + assertThatCode(() -> { + FieldWidth.u8(0, "f"); + FieldWidth.u8(255, "f"); + FieldWidth.u16(0, "f"); + FieldWidth.u16(65535, "f"); + FieldWidth.s8(-128, "f"); + FieldWidth.s8(127, "f"); + FieldWidth.bits(0, 7, "f"); + FieldWidth.bits(127, 7, "f"); + FieldWidth.bits(511, 9, "f"); + }).doesNotThrowAnyException(); + } + + @Test + void oneStepPastEachBoundaryIsRejected() + { + assertThatThrownBy(() -> FieldWidth.u8(256, "f")).isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> FieldWidth.u8(-1, "f")).isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> FieldWidth.u16(65536, "f")).isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> FieldWidth.u16(-1, "f")).isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> FieldWidth.s8(128, "f")).isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> FieldWidth.s8(-129, "f")).isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> FieldWidth.bits(128, 7, "f")).isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> FieldWidth.bits(512, 9, "f")).isInstanceOf(IllegalArgumentException.class); + } + + @Test + void theFailureNamesTheFieldTheValueAndTheLimit() + { + // a message that omits any of the three cannot be acted on: the user needs to know + // which cell, what they typed, and what would have been acceptable + assertThatThrownBy(() -> FieldWidth.bits(512, 9, "Move ID")) + .hasMessageContaining("Move ID") + .hasMessageContaining("512") + .hasMessageContaining("511"); + } + } + + @Nested + @DisplayName("learnset packing") + class LearnsetPacking + { + /** an empty level-up learnset: just the 0xFFFF terminator and its padding */ + private LearnsetData empty() + { + return new LearnsetData(new BytesDataContainer(GameFiles.LEVEL_UP_LEARNSETS, null, + new byte[] {(byte) 0xFF, (byte) 0xFF, 0, 0})); + } + + private LearnsetData withOneEntry(int moveID, int level) + { + LearnsetData data = empty(); + data.add(new LearnsetData.LearnsetEntry(moveID, level)); + return data; + } + + @Test + void anEntryThatFitsSurvivesSaveAndReload() + { + // the property that matters: what was set is what comes back. asserted at the + // extremes of both fields, where a mask would bite first + // (511, 127) is deliberately absent: it is individually in range but packs to + // 0xFFFF, the terminator, and is rejected. It is covered by its own test below. + for (int[] pair : new int[][] {{0, 0}, {511, 126}, {510, 127}, {1, 1}, {350, 100}}) + { + LearnsetData saved = withOneEntry(pair[0], pair[1]); + LearnsetData reloaded = empty(); + reloaded.setData(saved.save()); + + assertThat(reloaded).as("learnset holding move %d at level %d", pair[0], pair[1]) + .hasSize(1); + assertThat(reloaded.get(0).getMoveID()).isEqualTo(pair[0]); + assertThat(reloaded.get(0).getLevel()).isEqualTo(pair[1]); + } + } + + @Test + void aMoveIdPastNineBitsIsRefusedRatherThanTruncated() + { + // 512 & 0x1FF == 0, so this used to save as "no move" with nothing reported. + // the test asserts the failure, and that the message names the value the user typed + assertThatThrownBy(() -> withOneEntry(512, 5).save()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("512"); + } + + @Test + void aLevelPastSevenBitsIsRefusedRatherThanTruncated() + { + // 200 & 0x7F == 72 + assertThatThrownBy(() -> withOneEntry(1, 200).save()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("200"); + } + + @Test + void theCombinationThatCollidesWithTheTerminatorIsRefused() + { + // (127 << 9) | 511 == 0xFFFF, which is exactly the end-of-list marker. both fields + // are individually legal, so only the packed value reveals the problem - saving it + // wrote a terminator and the entry silently disappeared on reload + assertThatThrownBy(() -> withOneEntry(511, 127).save()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("511") + .hasMessageContaining("127"); + } + + @Test + void everyOtherInRangeCombinationRoundTrips() + { + // exhaustive over both fields: the terminator collision must be the ONLY hole. + // a sampled test would have missed it, and would miss a second one just as easily + for (int level = 0; level <= 127; level++) + { + for (int move = 0; move <= 511; move++) + { + if (((level << 9) | move) == 0xFFFF) + continue; + + LearnsetData reloaded = empty(); + reloaded.setData(withOneEntry(move, level).save()); + assertThat(reloaded).as("move %d at level %d", move, level).hasSize(1); + assertThat(reloaded.get(0).getMoveID()).as("move %d at level %d", move, level).isEqualTo(move); + assertThat(reloaded.get(0).getLevel()).as("move %d at level %d", move, level).isEqualTo(level); + } + } + } + + @Test + void aNegativeMoveIdIsRefused() + { + // reachable from the sheet: a combo box with nothing selected reports -1, and + // -1 & 0x1FF is 511 - a real move, silently substituted for an empty selection + assertThatThrownBy(() -> withOneEntry(-1, 5).save()) + .isInstanceOf(IllegalArgumentException.class); + } + } +} From 0da566f7808f53b1a3762eb8893d3569f7e18727 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 20:43:34 +0000 Subject: [PATCH 08/13] Correct the Pokemon type bound to 0..17 Generation 4 defines 18 types, numbered 0 to 17, and the frontend's typeColors holds exactly 18 entries - so 18 is one past the end, not the last valid value. The guard admitted a type that nothing could draw. Named as a constant so the frontend can derive its cell range from it rather than restating the number, which is how the two drifted in the first place. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob --- .../formats/personal/PersonalData.java | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java index 9772f58..d2508c0 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java @@ -327,6 +327,12 @@ public void setSpDef(int spDef) this.spDef = spDef; } + /** + * How many Pokemon types the game defines. Generation 4 has 18, numbered 0 to 17; a ROM + * with expanded types would need this and PokeditorManager.typeColors raised together. + */ + public static final int NUMBER_OF_TYPES = 18; + public int getType1() { return type1; @@ -334,8 +340,12 @@ public int getType1() public void setType1(int type1) { - if (type1 >= 19) // TODO have a way to know for certain the number of types - throw new RuntimeException("Maximum type value is 18. Provided: " + type1); + // Generation 4 defines 18 types, numbered 0 to 17, and PokeditorManager.typeColors + // holds exactly 18 entries - so 18 is one past the end, not the last valid value. The + // bound used to be 19, which admitted a type that nothing could draw. + if (type1 >= NUMBER_OF_TYPES) + throw new RuntimeException("Type values run from 0 to " + (NUMBER_OF_TYPES - 1) + + ". Provided: " + type1); this.type1 = type1; } @@ -346,8 +356,12 @@ public int getType2() public void setType2(int type2) { - if (type2 >= 19) // TODO have a way to know for certain the number of types - throw new RuntimeException("Maximum type value is 18. Provided: " + type2); + // Generation 4 defines 18 types, numbered 0 to 17, and PokeditorManager.typeColors + // holds exactly 18 entries - so 18 is one past the end, not the last valid value. The + // bound used to be 19, which admitted a type that nothing could draw. + if (type2 >= NUMBER_OF_TYPES) + throw new RuntimeException("Type values run from 0 to " + (NUMBER_OF_TYPES - 1) + + ". Provided: " + type2); this.type2 = type2; } From 53215dd94c2c0e20a3572d749c05392096a05a18 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 20:48:22 +0000 Subject: [PATCH 09/13] Stop refusing data a hacked ROM legitimately contains Two guards rejected values the tool exists to edit. An evolution file's entry cap was fixed at the retail count of seven, while setData deliberately reads every entry the file contains and fileSize is kept at the input's length - both so an expanded table is not silently truncated. The result contradicted itself: a ROM with ten evolutions parsed cleanly and then threw on save, aborting the write of the entire NARC and losing every other file in it along with the edit. The cap now comes from the file's own size, so a file can always write back what it was read with, and a retail file still refuses an eighth entry. The Pokemon type setters threw above a hardcoded count. A type occupies a whole byte; the number of types a game defines is game data, not a storage limit, and ROM hacks that add types are exactly what this library is for. The byte width is still enforced at save, which is the real constraint. The sheet keeps its own bound - it can only name and colour the types it has entries for - but that belongs in the user interface, where the consequence is a value you cannot conveniently edit, rather than in the data layer, where it is a file you cannot open. This reverses the direction of the previous commit, which tightened that bound from 19 to 18. Correcting the off-by-one was right; enforcing it in Core at all was not. Also removes Game.getRegion() and the static field behind it. Replacing the per-constant region with a static one made the sharing worse, not better: every Game constant then answered with the region of whichever ROM was parsed last. The region travels in BaseRomInfo, which is what parseBaseRom returns, and the deprecated single-argument Tables.initialize that depended on the static had no callers left. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob --- .../formats/evolutions/EvolutionData.java | 23 +++++-- .../formats/personal/PersonalData.java | 21 ++---- .../turtleisaac/pokeditor/gamedata/Game.java | 19 +----- .../pokeditor/gamedata/Tables.java | 12 ---- .../pokeditor/formats/FieldWidthTest.java | 64 +++++++++++++++++++ 5 files changed, 93 insertions(+), 46 deletions(-) diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/evolutions/EvolutionData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/evolutions/EvolutionData.java index 19ca711..95e52da 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/evolutions/EvolutionData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/evolutions/EvolutionData.java @@ -36,7 +36,7 @@ public void setData(BytesDataContainer files) fileSize = Math.max(file.length, FIXED_FILE_SIZE); // everything the file holds is read - dropping entries here would silently discard them on save - int numEntries = file.length / 6; + int numEntries = file.length / ENTRY_SIZE; for (int i = 0; i < numEntries; i++) { // unsigned: these are species and item IDs, which run past 0x7FFF in expanded ROMs. @@ -52,9 +52,16 @@ public BytesDataContainer save() MemBuf dataBuf = MemBuf.create(); MemBuf.MemBufWriter writer = dataBuf.writer(); - if (size() > MAX_NUM_ENTRIES) + // the cap is what THIS file can hold, not what a retail one holds. setData deliberately + // reads every entry present so an expanded table is not silently truncated, and fileSize + // is kept at the input's length for the same reason - refusing to write those entries back + // would parse a hacked ROM cleanly and then abort the whole NARC on save. + int capacity = (fileSize - TERMINATOR_SIZE) / ENTRY_SIZE; + if (size() > capacity) { - throw new RuntimeException("An evolution file can hold at most " + MAX_NUM_ENTRIES + " entries. Provided: " + size()); + throw new RuntimeException("This evolution file holds " + capacity + " entries (" + + fileSize + " bytes). Provided: " + size() + + ". Remove an evolution, or expand the file first."); } for(EvolutionEntry entry : this) @@ -127,6 +134,14 @@ public void setResultSpecies(int resultSpecies) } } + /** bytes per entry: method, requirement and result species, each a u16 */ + public static final int ENTRY_SIZE = 6; + + /** the trailing u16 terminator every evolution file carries */ + public static final int TERMINATOR_SIZE = 2; + + /** how many entries a retail-sized file holds; an expanded file holds more */ public static final int MAX_NUM_ENTRIES = 7; - public static final int FIXED_FILE_SIZE = MAX_NUM_ENTRIES * 6 + 2; + + public static final int FIXED_FILE_SIZE = MAX_NUM_ENTRIES * ENTRY_SIZE + TERMINATOR_SIZE; } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java index d2508c0..4d25d53 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalData.java @@ -328,8 +328,13 @@ public void setSpDef(int spDef) } /** - * How many Pokemon types the game defines. Generation 4 has 18, numbered 0 to 17; a ROM - * with expanded types would need this and PokeditorManager.typeColors raised together. + * How many Pokemon types an unmodified Generation 4 game defines: 18, numbered 0 to 17. + *

+ * This is what retail data contains, not a limit on what the field can hold - the type is + * stored in a whole byte, and ROM hacks with more than 18 types are exactly the thing this + * library exists to edit. The setters therefore do not enforce it; only the byte width is + * enforced, at save. A user interface that can only name or colour 18 types should say so + * itself rather than have the data layer refuse the value. */ public static final int NUMBER_OF_TYPES = 18; @@ -340,12 +345,6 @@ public int getType1() public void setType1(int type1) { - // Generation 4 defines 18 types, numbered 0 to 17, and PokeditorManager.typeColors - // holds exactly 18 entries - so 18 is one past the end, not the last valid value. The - // bound used to be 19, which admitted a type that nothing could draw. - if (type1 >= NUMBER_OF_TYPES) - throw new RuntimeException("Type values run from 0 to " + (NUMBER_OF_TYPES - 1) - + ". Provided: " + type1); this.type1 = type1; } @@ -356,12 +355,6 @@ public int getType2() public void setType2(int type2) { - // Generation 4 defines 18 types, numbered 0 to 17, and PokeditorManager.typeColors - // holds exactly 18 entries - so 18 is one past the end, not the last valid value. The - // bound used to be 19, which admitted a type that nothing could draw. - if (type2 >= NUMBER_OF_TYPES) - throw new RuntimeException("Type values run from 0 to " + (NUMBER_OF_TYPES - 1) - + ". Provided: " + type2); this.type2 = type2; } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Game.java b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Game.java index b68ea65..8b83a3a 100755 --- a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Game.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Game.java @@ -24,14 +24,6 @@ public enum Game public final String[] sheetList; public final String[] editorList; - /** - * @deprecated the region of the ROM currently being worked with is not a property of the game, it is a - * property of the individual ROM. This only exists so that callers of the deprecated - * {@link #getRegion()} keep working, and will be removed alongside it - use the - * {@link BaseRomInfo} returned by {@link #parseBaseRom(String)} instead. - */ - @Deprecated - private static Region lastParsedRegion; Game(String[] sheetList, String[] editorList) { @@ -47,11 +39,6 @@ public enum Game * every ROM opened in this process. Use the {@link BaseRomInfo} returned by * {@link #parseBaseRom(String)} and pass its region around explicitly instead. */ - @Deprecated - public Region getRegion() - { - return lastParsedRegion; - } /** * The game and region identified by a base ROM's game code @@ -72,9 +59,9 @@ public static BaseRomInfo parseBaseRom(String baseRomGameCode) default -> throw new RuntimeException("Invalid game"); }; - Region region = Region.getRegion(baseRomGameCode.charAt(3)); - lastParsedRegion = region; - return new BaseRomInfo(game, region); + // the region travels with the result rather than being stashed anywhere: a field on the + // enum, static or per-constant, is shared by every ROM opened in the process + return new BaseRomInfo(game, Region.getRegion(baseRomGameCode.charAt(3))); } public enum Region diff --git a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Tables.java b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Tables.java index 6105bfa..3e61eff 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Tables.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Tables.java @@ -34,18 +34,6 @@ public int getPointerOffset() return pointerOffset; } - /** - * Initializes the table pointers for the given base ROM - * - * @param baseROM a Game - * @deprecated use {@link #initialize(Game, Game.Region)} - relying on - * {@link Game#getRegion()} means the region of a previously opened ROM can leak into this one - */ - @Deprecated - public static void initialize(Game baseROM) - { - initialize(baseROM, baseROM.getRegion()); - } public static void initialize(Game baseROM, Game.Region region) { diff --git a/src/test/java/io/github/turtleisaac/pokeditor/formats/FieldWidthTest.java b/src/test/java/io/github/turtleisaac/pokeditor/formats/FieldWidthTest.java index 2794741..e52c093 100644 --- a/src/test/java/io/github/turtleisaac/pokeditor/formats/FieldWidthTest.java +++ b/src/test/java/io/github/turtleisaac/pokeditor/formats/FieldWidthTest.java @@ -1,5 +1,6 @@ package io.github.turtleisaac.pokeditor.formats; +import io.github.turtleisaac.pokeditor.formats.evolutions.EvolutionData; import io.github.turtleisaac.pokeditor.formats.learnsets.LearnsetData; import io.github.turtleisaac.pokeditor.gamedata.GameFiles; import org.junit.jupiter.api.DisplayName; @@ -81,6 +82,69 @@ void theFailureNamesTheFieldTheValueAndTheLimit() } } + @Nested + @DisplayName("expanded files") + class Expanded + { + /** + * Users of this tool routinely open ROMs with tables larger than retail. setData reads + * every entry a file contains, on purpose, so save has to be able to write them back. + * A cap fixed at the retail count parses such a file cleanly and then aborts the whole + * NARC write, which loses the edit and every other file in it. + */ + private EvolutionData evolutionsFromFileOf(int entries) + { + byte[] file = new byte[entries * EvolutionData.ENTRY_SIZE + EvolutionData.TERMINATOR_SIZE]; + return new EvolutionData(new BytesDataContainer(GameFiles.EVOLUTIONS, null, file)); + } + + @Test + void aRetailSizedEvolutionFileRoundTrips() + { + EvolutionData data = evolutionsFromFileOf(EvolutionData.MAX_NUM_ENTRIES); + assertThat(data).hasSize(EvolutionData.MAX_NUM_ENTRIES); + assertThatCode(data::save).doesNotThrowAnyException(); + } + + @Test + void anExpandedEvolutionFileSavesEveryEntryItWasReadWith() + { + // the property: whatever setData accepted, save must be able to write. Anything else + // means the file could be opened but not closed again. + for (int entries : new int[] {8, 10, 20, 50}) + { + EvolutionData data = evolutionsFromFileOf(entries); + assertThat(data).as("a file sized for %d entries", entries).hasSize(entries); + assertThatCode(data::save) + .as("a file sized for %d entries must save the %d entries it was read with", + entries, entries) + .doesNotThrowAnyException(); + } + } + + @Test + void addingMoreEntriesThanTheFileHoldsIsStillRefused() + { + // the cap is not gone, it is derived: a retail file still cannot take an eighth + // evolution, and the message says how many it holds rather than quoting a constant + EvolutionData data = evolutionsFromFileOf(EvolutionData.MAX_NUM_ENTRIES); + data.add(new EvolutionData.EvolutionEntry(1, 2, 3)); + + assertThatThrownBy(data::save) + .isInstanceOf(RuntimeException.class) + .hasMessageContaining(String.valueOf(EvolutionData.MAX_NUM_ENTRIES)); + } + + @Test + void anExpandedFileRefusesOnlyPastItsOwnCapacity() + { + EvolutionData data = evolutionsFromFileOf(20); + data.add(new EvolutionData.EvolutionEntry(1, 2, 3)); + + assertThatThrownBy(data::save).isInstanceOf(RuntimeException.class); + } + } + @Nested @DisplayName("learnset packing") class LearnsetPacking From a60bba40ebaa601af2033ef35cf059dea30ebc16 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 07:11:17 +0000 Subject: [PATCH 10/13] Correct two justifications that do not hold, and drop a dangling javadoc The comment on PersonalParser's fields said the state could not be static because the parser is a singleton and ROM A's TM table would otherwise be written into ROM B. The reasoning is backwards: being a singleton is exactly why instance fields buy no isolation - the parser is bound with Scopes.SINGLETON, so there is one instance for the lifetime of the process and its fields are as process-global as the static ones were. What actually keeps one ROM's data out of another is the frontend discarding its caches when the ROM changes. Same claim, same correction, in PokemonSpriteParser. The change itself is worth keeping - per-parser state belongs on the parser - so only the justification is rewritten. Also removes the javadoc left behind when getRegion() was deleted; it had been sitting above BaseRomInfo's own javadoc, documenting nothing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob --- .../pokeditor/formats/personal/PersonalParser.java | 7 +++++-- .../formats/pokemon_sprites/PokemonSpriteParser.java | 4 ++-- .../io/github/turtleisaac/pokeditor/gamedata/Game.java | 8 -------- 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalParser.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalParser.java index 0a85ae3..55322fb 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalParser.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/personal/PersonalParser.java @@ -38,8 +38,11 @@ public class PersonalParser implements GenericParser { - // this parser is a singleton, so none of this may be static - otherwise the TM/HM table read out of - // ROM A would be written into ROM B + // Instance state rather than static, which is where per-parser state belongs. Note this + // buys no isolation between ROMs on its own: the parser is bound as a Guice singleton, so + // there is exactly one instance for the lifetime of the process and instance fields are as + // process-global as static ones were. What actually keeps one ROM's data out of another is + // DataManager discarding its caches when the ROM changes. private final int[] tmMoveIdNumbers = new int[PersonalData.NUMBER_TMS_HMS]; private final int[] tmMoveTypes = new int[PersonalData.NUMBER_TMS_HMS]; // the palette index exactly as it was read, so that indices this editor does not recognize survive a save diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java index b75ee4b..e19296f 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java @@ -24,8 +24,8 @@ public class PokemonSpriteParser implements GenericParser private static final int NUM_PARTY_ICON_STARTING_FILES = 7; - // this parser is a singleton, so these must NOT be static - otherwise the files read out of ROM A - // would be written into ROM B + // Instance state rather than static. As in PersonalParser, this is not what isolates one + // ROM from another - the parser is a Guice singleton - it is simply where the state belongs. private List partyIconStartingFiles; private int partyIconPaletteTableLength = -1; diff --git a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Game.java b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Game.java index 8b83a3a..3daa42e 100755 --- a/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Game.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/gamedata/Game.java @@ -31,14 +31,6 @@ public enum Game this.editorList= editorList; } - /** - * Gets the region of the most recently parsed base ROM. - * - * @return a Region, or null if no base ROM has been parsed yet - * @deprecated the region belongs to the ROM, not to the Game constant, which is shared by - * every ROM opened in this process. Use the {@link BaseRomInfo} returned by - * {@link #parseBaseRom(String)} and pass its region around explicitly instead. - */ /** * The game and region identified by a base ROM's game code From fdc29569284986e467f918dd983fe60893aaaa81 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 23:51:45 +0000 Subject: [PATCH 11/13] Declare the Jackson this module uses, drop what it does not Removed jackson-dataformat-xml and junit:junit, neither of which is imported anywhere here - this module is entirely on JUnit 5. Removing the xml artifact broke the build, which is the useful part: nothing used it directly, but it was the only route to jackson-databind, which the text and script parsers do use. Declared explicitly now, along with jackson-core, because a module should declare what it imports rather than inherit it by accident from something unrelated. Guice moves to test scope. Nothing in src/main imports it - the two mentions are comments about how the consumer binds these parsers - and the injector that uses it lives in this module's tests. At compile scope it sat on the compile classpath of every module downstream for no reason. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RqYGZhtVb8KrPA9ftp5bob --- pom.xml | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/pom.xml b/pom.xml index 3038dbe..ca2efca 100644 --- a/pom.xml +++ b/pom.xml @@ -16,6 +16,22 @@ + + + com.fasterxml.jackson.core + jackson-databind + 2.15.2 + + + + com.fasterxml.jackson.core + jackson-core + 2.15.2 + + org.junit.jupiter junit-jupiter @@ -32,18 +48,11 @@ com.google.inject guice 7.0.0 - - - junit - junit - 4.13.2 + test - - com.fasterxml.jackson.dataformat - jackson-dataformat-xml - 2.15.2 - org.antlr From 0056b425201b6f2aaaec428b98bdfc9318dec61e Mon Sep 17 00:00:00 2001 From: turtleisaac Date: Thu, 27 Aug 2026 02:12:43 -0700 Subject: [PATCH 12/13] Fix field-script and sprite-palette round-trip corruption Verified against retail Platinum, HeartGold and SoulSilver. - FieldScriptData: stop treating endstd (0x15) as a run terminator. It is a zero-parameter fall-through, and terminating on it dropped the commands that followed. Per-file round-trip on HG/SS: one script recovers, zero regressions. - CommandMacro/CommandWriter: don't reject a null parameter up front for variable-length (.if-guarded) command macros, where a conditional branch may legitimately skip it. Defer the "not provided" error to the point the writer actually emits the parameter. Fixes save() throwing on UnionGroup/MysteryGiftGive/Strength in retail HG/SS. - PokemonSpriteData/PokemonSpriteParser: read each palette's own bit depth (0) instead of forcing 4, which mislabelled 8bpp palettes (the 0x4 marker was re-emitted as 0x3). BATTLE_SPRITES now round-trips 2964/2964 byte-exact. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../pokemon_sprites/PokemonSpriteData.java | 6 ++++-- .../pokemon_sprites/PokemonSpriteParser.java | 2 +- .../formats/scripts/FieldScriptData.java | 6 ++++-- .../formats/scripts/antlr4/CommandMacro.java | 8 +++++-- .../formats/scripts/antlr4/CommandWriter.java | 21 +++++++++++++++++-- 5 files changed, 34 insertions(+), 9 deletions(-) diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteData.java index 520617b..7dd46d4 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteData.java @@ -114,10 +114,12 @@ public void setData(BytesDataContainer files) paletteEmpty = paletteFile.length == 0; shinyPaletteEmpty = shinyPaletteFile.length == 0; + // 0 = read the file's own bit depth. Forcing 4 mislabels the occasional 8bpp palette + // (its 0x4 marker gets re-emitted as 0x3), corrupting it on a plain load/save round-trip. if (!paletteEmpty) - palette = new Palette(paletteFile, 4); + palette = new Palette(paletteFile, 0); if (!shinyPaletteEmpty) - shinyPalette = new Palette(shinyPaletteFile, 4); + shinyPalette = new Palette(shinyPaletteFile, 0); if (femaleBackFile.length != 0) femaleBack = new IndexedImage(femaleBackFile, 0, 0, 1, 1, scanFrontToBack); diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java index e19296f..d75af38 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java @@ -91,7 +91,7 @@ public List generateDataList(Map narcs, Map< } partyIconPaletteTableLength = partyIconPaletteIndices.length; - Palette partyIconPalette = new Palette(partyIcons.getFile(0), 4); + Palette partyIconPalette = new Palette(partyIcons.getFile(0), 0); // read the file's own bit depth partyIconStartingFiles = new ArrayList<>(); for (int i = 0; i < NUM_PARTY_ICON_STARTING_FILES; i++) { diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/FieldScriptData.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/FieldScriptData.java index 1151af8..02c4b56 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/FieldScriptData.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/FieldScriptData.java @@ -17,8 +17,10 @@ public class FieldScriptData extends GenericScriptData // 225 (goto_if_trainer_defeated) is a relative branch, so its destination has to be registered as a label private static final IntPredicate isCallCommand = commandID -> (commandID >= 0x16 && commandID <= 0x1D && commandID != 0x1B) || commandID == GOTO_IF_TRAINER_DEFEATED; - // 0x15 is endstd (yield to parent context), which terminates the current run of commands just like end/goto/return - private static final IntPredicate isEndCommand = commandId -> commandId == 0x2 || commandId == 0x15 || commandId == 0x16 || commandId == 0x1B; + // 0x2 (End), 0x16 (Jump) and 0x1B (Return) terminate the current run of commands. + // 0x15 (endstd/LocalScript) does NOT: verified against retail HeartGold and SoulSilver, treating it as a + // terminator drops the commands that follow it (round-trips one fewer script and re-emits shorter files). + private static final IntPredicate isEndCommand = commandId -> commandId == 0x2 || commandId == 0x16 || commandId == 0x1B; // 225 is NOT a comparator command - per Scrcmd_Hg.txt it takes a single relative destination and no comparator byte private static final IntPredicate isDoIfCommand = commandID -> commandID == 28 || commandID == 29; private static final IntPredicate isMovementCommand = commandID -> commandID == 0x5E; diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandMacro.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandMacro.java index 21ac8ad..aaf30af 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandMacro.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandMacro.java @@ -57,7 +57,11 @@ public void write(MemBuf memBuf, CommandWriter.LabelOffsetObtainer offsetObtaine Object param = parameterValues[idx++]; if (param == null) { - throw new RuntimeException(String.format("The parameter \"%s\" of the command \"%s\" was not provided a value", parameter, name)); + // A conditional/variable-length macro (.if-guarded, defaulted args) legitimately leaves + // some declared parameters unread, so a null here is not necessarily an error. The writer + // only visits a parameter when its branch is actually taken, and raises a precise error at + // that point if a genuinely-required parameter turns out to be missing. See CommandWriter. + continue; } else if (param instanceof Number number) parameterToValueMap.put(parameter, number); @@ -94,7 +98,7 @@ else if (parameterValues != null && parameterValues.length != 0) throw new RuntimeException(String.format("The command \"%s\" takes no parameters but %d were provided", name, parameterValues.length)); } - CommandWriter commandWriter = new CommandWriter(memBuf.writer(), offsetObtainer, parameterToValueMap); + CommandWriter commandWriter = new CommandWriter(memBuf.writer(), offsetObtainer, parameterToValueMap, this); commandWriter.visitEntry(entryContext); } diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandWriter.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandWriter.java index ba3487b..13ad53c 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandWriter.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/scripts/antlr4/CommandWriter.java @@ -27,11 +27,17 @@ public class CommandWriter extends CommandMacroVisitor private int parameterType; private boolean compareMode; - public CommandWriter(MemBuf.MemBufWriter writer, LabelOffsetObtainer offsetObtainer, Map parameterToValueMap) + public CommandWriter(MemBuf.MemBufWriter writer, LabelOffsetObtainer offsetObtainer, Map parameterToValueMap, CommandMacro macro) { this.writer = writer; this.parameterToValueMap = parameterToValueMap; this.offsetObtainer = offsetObtainer; + this.macro = macro; + } + + private String commandName() + { + return macro != null ? macro.getName() : "?"; } @Override @@ -73,6 +79,12 @@ public Integer visitTerminal(TerminalNode node) if (!compareMode) { // writing arg value to file Object value = parameterToValueMap.get(text); + if (value == null) + { + // This parameter is actually being emitted but the reader never gave it a value - + // a genuinely required parameter is missing (as opposed to one a .if branch skips). + throw new RuntimeException(String.format("The parameter \"%s\" of the command \"%s\" was not provided a value", text, commandName())); + } int valueToWrite; if (value instanceof Number number) @@ -90,7 +102,12 @@ public Integer visitTerminal(TerminalNode node) } else // this is a calculation which requires an already read value { - return (int) parameterToValueMap.get(text); + Object value = parameterToValueMap.get(text); + if (value == null) + { + throw new RuntimeException(String.format("The parameter \"%s\" of the command \"%s\" was used in a condition but not provided a value", text, commandName())); + } + return (int) value; } } else if (terminalNode.symbol.getType() == MacrosLexer.NUMBER) { return Integer.decode(terminalNode.getText()); From 0a34a35e426b0ddbbbee4fdda33799d8366e5e8d Mon Sep 17 00:00:00 2001 From: turtleisaac Date: Thu, 27 Aug 2026 02:37:33 -0700 Subject: [PATCH 13/13] Preserve trailing party-icon subfiles on sprite save The PARTY_ICONS narc is [leading files] + [one icon per species] + [trailing files]. The parser only read the leading and per-species blocks, so saving a ROM dropped the trailing icons (alternate forms, egg/substitute, etc.) - the narc's subfile count fell from 547 to 501 on Platinum (551->501 on HG/SS). Capture the trailing block in generateDataList and re-append it in processDataList, mirroring the existing leading-files handling. Combined with the Nds4j NCGR character-header fidelity fix, the whole PARTY_ICONS narc now round-trips byte-for-byte (547/547 Platinum, 551/551 HeartGold/SoulSilver), and PokemonSpriteTests passes byte-exact on all three retail ROMs. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../pokemon_sprites/PokemonSpriteParser.java | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java b/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java index d75af38..6107a57 100644 --- a/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java +++ b/src/main/java/io/github/turtleisaac/pokeditor/formats/pokemon_sprites/PokemonSpriteParser.java @@ -27,6 +27,7 @@ public class PokemonSpriteParser implements GenericParser // Instance state rather than static. As in PersonalParser, this is not what isolates one // ROM from another - the parser is a Guice singleton - it is simply where the state belongs. private List partyIconStartingFiles; + private List partyIconTrailingFiles; private int partyIconPaletteTableLength = -1; @Override @@ -124,6 +125,15 @@ public List generateDataList(Map narcs, Map< data.add(species); } + // Party icons after the per-species block (alternate forms, egg/substitute icons, etc.) are not part + // of the per-species editing model. Preserve them verbatim, as with the leading files, so that loading + // and saving a ROM round-trips the whole narc instead of dropping them. + partyIconTrailingFiles = new ArrayList<>(); + for (int i = NUM_PARTY_ICON_STARTING_FILES + numSpecies; i < partyIcons.getFiles().size(); i++) + { + partyIconTrailingFiles.add(partyIcons.getFile(i)); + } + return data; } @@ -159,9 +169,9 @@ public Map processDataList(List data, Map processDataList(List data, Map