From 40bb1b71b7ac655cceeb0a8c78d763e31b2c671f Mon Sep 17 00:00:00 2001 From: Algent Date: Sat, 15 Aug 2026 17:04:41 +0200 Subject: [PATCH] refactor: port PacketScheduler to Java IScheduledPacketPart is descriptor-identical and schedulePacket is unchanged. TScheduledPacketPart.read stays abstract: TMultiPart declares read, so a default would be shadowed and a part would silently read a description instead of a mask. The dispatch that lived in the $class helper is now the interface static readMask, with the one-line forwarder documented on read. writeScheduled and readScheduled became defaults. TMultiPart declares neither, and empty bodies are what mixing the Scala trait in already gave a Scala implementor -- only Java implementors ever had to write them out. sendScheduled is public because the reference's private[multipart] reaches the handler package and Java has no equivalent scope. An unsupported mask width now throws IllegalArgumentException rather than scala.MatchError; both are RuntimeException and the map is still left uncleared on that path, as before. Dropped PacketScheduler$, TScheduledPacketPart$class and two closure classes, none referenced in bytecode or by reflective name. 118 plain-JVM tests and 18 Forge server tests pass. --- JAVA_MIGRATION.md | 6 ++ JAVA_MIGRATION_DIVERGENCES.md | 59 ++++++++++++++ JAVA_MIGRATION_HANDOFF.md | 22 +++-- .../test/PacketSchedulerFunctionalTest.java | 2 +- .../multipart/IScheduledPacketPart.java | 22 +++++ .../multipart/PacketScheduler.java | 64 +++++++++++++++ .../multipart/PacketScheduler.scala | 80 ------------------ .../multipart/TScheduledPacketPart.java | 54 +++++++++++++ .../PacketSchedulerCharacterizationTest.java | 81 +++++++++++-------- 9 files changed, 267 insertions(+), 123 deletions(-) create mode 100644 src/main/scala/codechicken/multipart/IScheduledPacketPart.java create mode 100644 src/main/scala/codechicken/multipart/PacketScheduler.java delete mode 100644 src/main/scala/codechicken/multipart/PacketScheduler.scala create mode 100644 src/main/scala/codechicken/multipart/TScheduledPacketPart.java diff --git a/JAVA_MIGRATION.md b/JAVA_MIGRATION.md index 558c9c5..aee2165 100644 --- a/JAVA_MIGRATION.md +++ b/JAVA_MIGRATION.md @@ -524,3 +524,9 @@ These ranges were formed before designing the characterization suite and should - Dropped `TileCache$`, `TileCache$FlaggedTile$` and the whole case-class machinery on `FlaggedTile`. It is only ever a map value, never compared, hashed, printed or destructured. This is the `IconHitEffects$` case rather than the `MultipartHelper$` one: nothing names it in bytecode or by string. - Pinned an unreachable branch before removing it. The reference's `findTile` ends in `case _ => null`, but `Some(FlaggedTile(t, rem))` matches whatever the flag holds, so a tile flagged removed is returned like any other and only the warning is suppressed. The functional test asserts that before the port so the Java version cannot quietly start returning null. - Confirmed the adapter approach works for ports that change collection types. The characterization test named `scala.Option` in exactly one helper, so the port's test diff was two lines and every behavioral assertion carried over untouched. +- Ported `PacketScheduler`, `IScheduledPacketPart` and `TScheduledPacketPart`. `IScheduledPacketPart` is descriptor-identical, `schedulePacket` is unchanged, and `TScheduledPacketPart.read` stays abstract because `TMultiPart` declares it. +- Found the case the default-versus-abstract rule splits within a single interface. `read` cannot be a default because `TMultiPart` declares it, but `writeScheduled` and `readScheduled` can, because `TMultiPart` declares neither. Making the latter two defaults restores what a Scala implementor already got from the trait for free; only Java implementors ever had to write them out. +- Added `TScheduledPacketPart.readMask` as a Java 8 interface static, carrying the dispatch `TScheduledPacketPart$class.read` used to hold. Where a trait's central member cannot become a default, an interface static plus a documented one-line forwarder is the replacement, and it is the pattern to reuse. +- Widened `sendScheduled` from `private[multipart]` to public. Its only caller is in `codechicken.multipart.handler`, and Java has no scope spanning a package and its siblings, so package-private would not compile. Expect this wherever `private[multipart]` crosses into the handler package. +- Recorded that the whole subsystem has no implementors at all, in this codebase or in the pack. It is a third-party extension point, which is why every behavioral test had to construct its own part. +- Confirmed the Forge server suite can drive the packet path without players connected. `MultipartSPH.getTileStream` only needs a non-remote world and a position, so a part bound to a tile with `setWorldObj` is enough to exercise `getWriteStream`. diff --git a/JAVA_MIGRATION_DIVERGENCES.md b/JAVA_MIGRATION_DIVERGENCES.md index 282a97c..0ce547c 100644 --- a/JAVA_MIGRATION_DIVERGENCES.md +++ b/JAVA_MIGRATION_DIVERGENCES.md @@ -1010,3 +1010,62 @@ All of it is unreferenced, and each change removes a Scala type from a signature lines. Every behavioral assertion is unchanged and passed before and after. - What stays manual: the recovery path itself. It only fires when a client tile is evicted from its chunk before its update packet arrives, which no test reproduces. + +## 2026-08-15 — PacketScheduler Java port + +`PacketScheduler`, `IScheduledPacketPart` and `TScheduledPacketPart`. **Zero downstream references** and, unusually, +zero implementors in this codebase either: the whole subsystem is a third-party extension point that no shipping mod +in the pack uses. + +### Observable behavior + +No known divergence. `schedulePacket` still refuses a client world with `IllegalArgumentException`, still ORs repeated +masks for the same part, and `sendScheduled` still skips parts whose tile is null, still writes the mask at the part's +own width before calling `writeScheduled`, and still clears the schedule at the end. + +It also still fails rather than writing nothing for a mask width outside 1, 2, 4 and 8. The reference matched with no +fallback and raised `scala.MatchError`; the port throws `IllegalArgumentException`. Both are `RuntimeException`, which +is what the characterization asserts, and there is no consumer to notice the type. The map is still **not** cleared +when that throw happens, because the reference had no `try`/`finally` and the port does not add one. + +### Supported JVM API + +- `IScheduledPacketPart` is member- and descriptor-identical to the reference. +- `PacketScheduler.schedulePacket(TMultiPart, long)` is unchanged. +- `TScheduledPacketPart` keeps `read` **abstract**. `TMultiPart` declares `read`, so a default would be shadowed by the + superclass and a part would silently read a description instead of a mask. + +### Changed API + +`TScheduledPacketPart.writeScheduled` and `readScheduled` became interface defaults with empty bodies. They were +abstract on the reference interface with the empty bodies held in `TScheduledPacketPart$class`, which is to say a Scala +implementor already got them for free and only a Java implementor had to write them out. Making them defaults restores +the trait's source-level semantics for Java implementors rather than changing them. `TMultiPart` declares neither, so +the default-versus-superclass rule permits it. + +Added `TScheduledPacketPart.readMask(TScheduledPacketPart, MCDataInput)`, a Java 8 interface static carrying the mask +dispatch that `TScheduledPacketPart$class.read` used to hold. Because `read` cannot be a default, this is what makes +the interface worth mixing in: implementors write a one-line `read` that delegates to it. The javadoc carries the +snippet. + +`sendScheduled` is public. The reference declared it `private[multipart]`, which reaches `codechicken.multipart.handler` +where the only caller lives; Java has no scope that spans a package and its siblings, so package-private would not +compile and public is the only option. + +### Removed API + +`PacketScheduler$` with its `MODULE$`, and `TScheduledPacketPart$class`. Neither is referenced by any jar in the pack, +in bytecode or by reflective name, so both fall under the `IconHitEffects$` case rather than the `MultipartHelper$` +one. The two `PacketScheduler$$anonfun$` closure classes go with them; the Java loop produces no replacement. + +### Validation + +- Complete plain-JVM suite: 118 tests, 0 failures, 0 errors. +- Clean `spotlessApply checkstyleTest build`: passing. +- Java 8 Forge dedicated-server suite: 18 tests, 0 failures, 0 errors, 5 of them new for this port. +- Class list and members diffed against a reference jar built at `3ad25f5`. +- Two characterization adjustments, both anticipated: the functional test's `sendScheduled` helper stopped naming + `PacketScheduler$.MODULE$`, and the structural test stopped asserting that `writeScheduled` and `readScheduled` are + abstract on `TScheduledPacketPart`, asserting the member set instead. Every behavioral assertion is unchanged. +- What stays manual: the client-world guard. `schedulePacket` throws only when `world.isRemote`, which neither harness + can produce — the plain JVM cannot build a `World` and the dedicated server is never remote. diff --git a/JAVA_MIGRATION_HANDOFF.md b/JAVA_MIGRATION_HANDOFF.md index a3f3d0e..d5b64f1 100644 --- a/JAVA_MIGRATION_HANDOFF.md +++ b/JAVA_MIGRATION_HANDOFF.md @@ -10,7 +10,7 @@ what breaks, and what is left. The other documents hold the reasoning. | `JAVA_MIGRATION_DIVERGENCES.md` | Every intentional difference from the reference, one entry per port | | `JAVA_MIGRATION_MANUAL_CHECKS.md` | What no automated test can cover, and must be checked by hand in a client | -Branch: `algent/java`. Base: `master`. 46 commits so far. +Branch: `algent/java`. Base: `master`. 48 commits so far. ## The one rule that matters @@ -56,7 +56,7 @@ awk -F'"' '/ map = new HashMap<>(); + + private PacketScheduler() {} + + /** Add bits to the current update mask for part. (binary OR) */ + public static void schedulePacket(TMultiPart part, long mask) { + if (part.world().isRemote) { + throw new IllegalArgumentException("Cannot use PacketScheduler on a client world"); + } + + Long current = map.get(part); + map.put(part, (current == null ? 0L : current) | mask); + } + + /** + * Writes every scheduled part and clears the schedule. + *

+ * The reference declared this {@code private[multipart]}, which reaches the handler package. Java has no equivalent + * scope, so it is public. + */ + public static void sendScheduled() { + for (Map.Entry e : map.entrySet()) { + TMultiPart part = e.getKey(); + long mask = e.getValue(); + if (part.tile() != null) { + IScheduledPacketPart ipart = (IScheduledPacketPart) part; + MCDataOutput w = part.getWriteStream(); + switch (ipart.maskWidth()) { + case 1: + w.writeByte((int) mask); + break; + case 2: + w.writeShort((int) mask); + break; + case 4: + w.writeInt((int) mask); + break; + case 8: + w.writeLong(mask); + break; + default: + throw new IllegalArgumentException("Invalid maskWidth: " + ipart.maskWidth()); + } + + ipart.writeScheduled(mask, w); + } + } + map.clear(); + } +} diff --git a/src/main/scala/codechicken/multipart/PacketScheduler.scala b/src/main/scala/codechicken/multipart/PacketScheduler.scala deleted file mode 100644 index 30bbd3f..0000000 --- a/src/main/scala/codechicken/multipart/PacketScheduler.scala +++ /dev/null @@ -1,80 +0,0 @@ -package codechicken.multipart - -import scala.collection.mutable.{Map => MMap} -import codechicken.lib.data.MCDataOutput -import codechicken.lib.data.MCDataInput - -/** Static class for packing update data. When a specific property of a part - * changes and needs sending to the client, a bit can be set in the mask. This - * bit can then be checked in the writeScheduled callback. This prevents - * sending multiple packets if the same property updates more than once per - * tick. - */ -object PacketScheduler { - private val map = MMap[TMultiPart, Long]() - - /** Add bits to the current update mask for part. (binary OR) - */ - def schedulePacket(part: TMultiPart, mask: Long) { - if (part.world.isRemote) - throw new IllegalArgumentException( - "Cannot use PacketScheduler on a client world" - ) - - map.put(part, map.getOrElse(part, 0L) | mask) - } - - private[multipart] def sendScheduled() { - map.foreach { e => - val (part, mask) = e - if (part.tile != null) { - val ipart = part.asInstanceOf[IScheduledPacketPart] - val w = part.getWriteStream - ipart.maskWidth match { - case 1 => w.writeByte(mask.toInt) - case 2 => w.writeShort(mask.toInt) - case 4 => w.writeInt(mask.toInt) - case 8 => w.writeLong(mask) - } - - ipart.writeScheduled(mask, w) - } - } - map.clear() - } -} - -/** Callback interface for PacketScheduler - */ -trait IScheduledPacketPart { - - /** Write scheduled data to the packet, mask is the cumulative mask from calls - * to schedulePacket - */ - def writeScheduled(mask: Long, packet: MCDataOutput) - - /** Returns the width (in bytes) of the data type required to hold all valid - * mask bits. Valid values are 1, 2, 4 and 8 - */ - def maskWidth: Int - - /** Read data matching mask. Estiablishes a method for subclasses to override. - * This should be called from read - */ - def readScheduled(mask: Long, packet: MCDataInput) -} - -trait TScheduledPacketPart extends TMultiPart with IScheduledPacketPart { - final override def read(packet: MCDataInput) { - val mask = maskWidth match { - case 1 => packet.readUByte - case 2 => packet.readUShort - case 4 => packet.readInt - case 8 => packet.readLong - } - readScheduled(mask, packet) - } - - def writeScheduled(mask: Long, packet: MCDataOutput) {} - def readScheduled(mask: Long, packet: MCDataInput) {} -} diff --git a/src/main/scala/codechicken/multipart/TScheduledPacketPart.java b/src/main/scala/codechicken/multipart/TScheduledPacketPart.java new file mode 100644 index 0000000..12f189a --- /dev/null +++ b/src/main/scala/codechicken/multipart/TScheduledPacketPart.java @@ -0,0 +1,54 @@ +package codechicken.multipart; + +import codechicken.lib.data.MCDataInput; +import codechicken.lib.data.MCDataOutput; + +/** + * Mixin interface for parts whose updates are batched through {@link PacketScheduler}. Implementors must also extend + * {@link TMultiPart}; the Scala trait declared that, but a trait extending a class is a bare interface in bytecode. + */ +public interface TScheduledPacketPart extends IScheduledPacketPart { + + /** + * This cannot be a default method: {@link TMultiPart} declares read, and a superclass method always beats an + * interface default, so the default would never run and the part would read a description instead of a mask. Every + * implementor must declare it and delegate to {@link #readMask(TScheduledPacketPart, MCDataInput)}. + * + * 

+     * 
+     * @Override
+     * public final void read(MCDataInput packet) {
+     *     TScheduledPacketPart.readMask(this, packet);
+     * }
+     *
+ */ + void read(MCDataInput packet); + + @Override + default void writeScheduled(long mask, MCDataOutput packet) {} + + @Override + default void readScheduled(long mask, MCDataInput packet) {} + + /** Reads a mask of the part's own width and hands it to readScheduled. */ + static void readMask(TScheduledPacketPart part, MCDataInput packet) { + long mask; + switch (part.maskWidth()) { + case 1: + mask = packet.readUByte(); + break; + case 2: + mask = packet.readUShort(); + break; + case 4: + mask = packet.readInt(); + break; + case 8: + mask = packet.readLong(); + break; + default: + throw new IllegalArgumentException("Invalid maskWidth: " + part.maskWidth()); + } + part.readScheduled(mask, packet); + } +} diff --git a/src/test/java/codechicken/multipart/PacketSchedulerCharacterizationTest.java b/src/test/java/codechicken/multipart/PacketSchedulerCharacterizationTest.java index b5577bf..d03ec9a 100644 --- a/src/test/java/codechicken/multipart/PacketSchedulerCharacterizationTest.java +++ b/src/test/java/codechicken/multipart/PacketSchedulerCharacterizationTest.java @@ -13,7 +13,6 @@ import java.util.TreeSet; import org.junit.jupiter.api.Test; import codechicken.lib.data.MCDataInput; -import codechicken.lib.data.MCDataOutput; /** * schedulePacket refuses a client world and everything else here needs a bound tile, so the behavior lives in the Forge @@ -26,28 +25,49 @@ class PacketSchedulerCharacterizationTest { void theCallbackInterfaceDeclaresThreeAbstractMembersAndExtendsNothing() { assertTrue(IScheduledPacketPart.class.isInterface()); assertEquals(0, IScheduledPacketPart.class.getInterfaces().length); - assertAllAbstract( - IScheduledPacketPart.class, - "writeScheduled(long,codechicken.lib.data.MCDataOutput)void", - "readScheduled(long,codechicken.lib.data.MCDataInput)void", - "maskWidth()int"); - } - @Test - void thePartTraitExtendsTheCallbackInterfaceAndAddsRead() { - assertTrue(TScheduledPacketPart.class.isInterface()); - assertArrayEqualsAsSet(TScheduledPacketPart.class.getInterfaces(), IScheduledPacketPart.class); - assertAllAbstract( - TScheduledPacketPart.class, - "read(codechicken.lib.data.MCDataInput)void", - "writeScheduled(long,codechicken.lib.data.MCDataOutput)void", - "readScheduled(long,codechicken.lib.data.MCDataInput)void"); + for (Method method : IScheduledPacketPart.class.getDeclaredMethods()) { + assertTrue(Modifier.isAbstract(method.getModifiers()), method + " must stay abstract"); + } + assertEquals( + new TreeSet<>( + Arrays.asList( + "writeScheduled(long,codechicken.lib.data.MCDataOutput)void", + "readScheduled(long,codechicken.lib.data.MCDataInput)void", + "maskWidth()int")), + instanceSignatures(IScheduledPacketPart.class)); } /** - * TMultiPart declares read, so a superclass method would beat any default the interface tried to supply. The trait - * kept its implementation in the $class helper, which only Scala implementors reach; a part that does not declare - * read resolves it to TMultiPart's, which reads a description rather than a mask. + * The two callbacks the trait supplied empty bodies for are now interface defaults, which is what mixing the Scala + * trait in already gave a Scala implementor. read stays abstract for the reason below. + */ + @Test + void thePartTraitExtendsTheCallbackInterfaceAndAddsRead() { + assertTrue(TScheduledPacketPart.class.isInterface()); + assertEquals( + new TreeSet<>(Arrays.asList(IScheduledPacketPart.class.getName())), + names(TScheduledPacketPart.class.getInterfaces())); + assertEquals( + new TreeSet<>( + Arrays.asList( + "read(codechicken.lib.data.MCDataInput)void", + "writeScheduled(long,codechicken.lib.data.MCDataOutput)void", + "readScheduled(long,codechicken.lib.data.MCDataInput)void")), + instanceSignatures(TScheduledPacketPart.class)); + } + + @Test + void theOptionalCallbacksDoNothingUnlessOverridden() { + SilentPart part = new SilentPart(); + + part.writeScheduled(0xffL, null); + part.readScheduled(0xffL, null); + } + + /** + * TMultiPart declares read, so a superclass method would beat any default the interface tried to supply. A part + * that does not declare read resolves it to TMultiPart's, which reads a description rather than a mask. */ @Test void readMustStayAbstractBecauseTMultiPartDeclaresIt() throws Exception { @@ -60,18 +80,17 @@ class PacketSchedulerCharacterizationTest { "An implementor that does not declare read falls back to TMultiPart, not to the interface"); } - private static void assertAllAbstract(Class type, String... expectedMembers) { - Set actual = new TreeSet<>(); + /** Declared instance methods only, so the static mask reader is not counted as part of the contract. */ + private static Set instanceSignatures(Class type) { + Set signatures = new TreeSet<>(); for (Method method : type.getDeclaredMethods()) { - assertTrue(Modifier.isAbstract(method.getModifiers()), method + " must stay abstract"); + if (Modifier.isStatic(method.getModifiers())) { + continue; + } assertTrue(Modifier.isPublic(method.getModifiers()), method + " must stay public"); - actual.add(signature(method)); + signatures.add(signature(method)); } - assertEquals(new TreeSet<>(Arrays.asList(expectedMembers)), actual); - } - - private static void assertArrayEqualsAsSet(Class[] actual, Class... expected) { - assertEquals(new TreeSet<>(names(expected)), new TreeSet<>(names(actual))); + return signatures; } private static Set names(Class[] types) { @@ -105,11 +124,5 @@ class PacketSchedulerCharacterizationTest { public int maskWidth() { return 1; } - - @Override - public void writeScheduled(long mask, MCDataOutput packet) {} - - @Override - public void readScheduled(long mask, MCDataInput packet) {} } }