feat: update content types to load from codecs#359
Conversation
docs: add loader registry refactor design spec refactor: split entry page and condition type registries refactor: split multiblock type registry build: quick play into a NeoForge world refactor: split state matcher type registry refactor: replace loader registry bootstrap refactor: move dispatch registries into registry package refactor: use class-owned ids in book datagen refactor: remove legacy registry shims fix: lazy initialize recursive stream codecs fix: restore legacy state matcher ids and fields chore: simplify fix: restore legacy multiblock matcher field names fix: preserve book entry ids across codecs refactor: make book entry codecs pure
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the data loading and serialization system to use Mojang's Codecs and StreamCodecs, replacing manual JSON parsing and network buffer handling. It introduces a comprehensive registry system for book components (entries, pages, conditions) and multiblocks, and enforces the use of fully qualified identifiers for all internal links and references. Feedback identifies a potential scalability issue in character key generation for sparse multiblocks and a discrepancy in entry ID handling where IDs derived from file paths might conflict with those explicitly defined in JSON. Additionally, the reviewer notes that the strict identifier requirement is a breaking change that increases verbosity for manual JSON authors.
| Map<String, StateMatcher> mapping = new LinkedHashMap<>(); | ||
| int index = 0; | ||
| for (var entry : grouped.entrySet()) { | ||
| char key = (char) (33 + index++); |
There was a problem hiding this comment.
The logic for generating character keys for the sparse multiblock mapping is limited to approximately 94 unique matchers before it starts using non-printable or potentially problematic characters (ASCII 33 to 126). While it is unlikely a single multiblock will exceed this limit, a more robust approach using a wider range or a different key generation strategy would improve scalability.
| var sourceEntryId = Identifier.fromNamespaceAndPath(entry.getKey().getNamespace(), Arrays.stream(pathParts).skip(2).collect(Collectors.joining("/"))); | ||
|
|
||
| BookErrorManager.get().getContextHelper().entryId = entryId; | ||
| BookErrorManager.get().getContextHelper().entryId = sourceEntryId; | ||
| //test if we should load the category at all | ||
| if (!this.testConditionOnLoad(entryId, entry.getValue(), this.registries)) { | ||
| if (!this.testConditionOnLoad(sourceEntryId, entry.getValue(), this.registries)) { | ||
| continue; | ||
| } | ||
|
|
||
| var bookEntry = this.loadEntry(entryId, entry.getValue(), this.books.get(bookId).autoAddReadConditions(), this.registries); | ||
| var bookEntry = this.loadEntry(entry.getValue(), this.registries); | ||
| BookErrorManager.get().getContextHelper().entryId = bookEntry.getId(); |
There was a problem hiding this comment.
There is a discrepancy between how entry IDs are handled. sourceEntryId is derived from the file path (the standard Minecraft way), but the bookEntry is loaded via a codec that expects an id field within the JSON itself. If these two IDs do not match, the entry will be registered under the ID specified in the JSON, which may lead to confusion when linking via resource locations. It is recommended to either validate that they match or remove the redundant id field from the JSON and set it from the resource location after loading.
| var bookLink = new BookLink(); | ||
| var parts = linkText.split("/", 2); //discard everything after / | ||
| bookLink.bookId = Identifier.tryParse(parts[0]); | ||
| bookLink.bookId = parseIdentifier(parts[0], "book", linkText); |
There was a problem hiding this comment.
The transition to parseIdentifier (which uses Codecs.tryParseStrictIdentifier) enforces fully qualified identifiers (e.g., namespace:path) for all links. This is a breaking change for existing books that relied on omitting the namespace for links within the same book. While datagen has been updated to accommodate this, manual JSON authors will find this significantly more verbose.
1 participant
No description provided.