Conversation
|
The build fails due to this |
There was a problem hiding this comment.
Pull request overview
This PR introduces the ability to reload Geyser's mappings without requiring a full server restart, supporting a more efficient development workflow for resource pack converters. The implementation adds a clear() method to registry classes and creates a new /geyser reloadmappings command.
Key Changes:
- Added
clear()method to registry classes to enable clearing of mappings - Created new
/geyser reloadmappingscommand for explicit mapping reloads - Integrated mapping reload functionality into the existing
/geyser reloadcommand
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| languages | Updated subproject commit reference |
| Registry.java | Implemented clear() method with type-specific clearing logic for Collections, Maps, and BitSets |
| IRegistry.java | Added default clear() method to interface with unsupported operation exception |
| DeferredRegistry.java | Implemented clear() that delegates to backing registry when loaded |
| ArrayRegistry.java | Implemented clear() using Arrays.fill() to null array elements |
| ReloadMappingsCommand.java | New command class implementing mapping reload logic with session disconnection |
| ReloadCommand.java | Updated to call reloadMappings() after standard reload |
| CommandRegistry.java | Registered the new ReloadMappingsCommand |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Clears The Underlying Mappings. | ||
| * Throws {@link UnsupportedOperationException} When The Registry Doesn't Support It. |
There was a problem hiding this comment.
Incorrect capitalization in Javadoc. Standard Javadoc style uses sentence case, not title case. Should be "Clears the underlying mappings" and "Throws {@link UnsupportedOperationException} when the registry doesn't support it."
| * Clears The Underlying Mappings. | |
| * Throws {@link UnsupportedOperationException} When The Registry Doesn't Support It. | |
| * Clears the underlying mappings. | |
| * Throws {@link UnsupportedOperationException} when the registry doesn't support it. |
| } | ||
|
|
||
| /** | ||
| * Clears The Mappings. |
There was a problem hiding this comment.
Incorrect capitalization in Javadoc. Standard Javadoc style uses sentence case, not title case. Should be "Clears the mappings."
| * Clears The Mappings. | |
| * Clears the mappings. |
| source.sendMessage(GeyserLocale.getPlayerLocaleString("geyser.commands.reloadmappings.success", source.locale())); | ||
| }, 10, TimeUnit.MILLISECONDS); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing Javadoc for this static method. Since this method is being called from another class (ReloadCommand), it should have documentation explaining its purpose, behavior (especially the registry clearing and repopulation steps), and any threading considerations.
| /** | |
| * Reloads all runtime mappings and related registries used by Geyser. | |
| * <p> | |
| * This method clears existing block, item, tag, custom skull, and related | |
| * registries and then repopulates them in a specific order to ensure that | |
| * all dependencies (such as collisions, custom blocks, and potion mixes) | |
| * are initialized consistently. | |
| * <p> | |
| * This operation mutates global registry state and is not thread-safe; | |
| * it is intended to be invoked from the Geyser scheduled thread, as done | |
| * in {@link #execute(org.incendo.cloud.context.CommandContext)} via the | |
| * plugin's scheduler, and should not run concurrently with other registry | |
| * initialization or access code. | |
| */ |
|
What is the motivation behind these changes? I'm generally quite curious as to what this allows and how this feature would be used before I do a review, since at a glance it seems quite niche. I know you mention resource pack workflows, but I'm curious how that all works and where this falls into that. |
|
This implementation is pretty hacky as is, it can result in a state where a geyser instance is already joinable, but doesn't have block/item mappings loaded yet; which will lead to issues I'm all for making these reloadable - although IMO it should just be part of the reload command since dc'ing all players is needed anyways - but this should be done as part of the reload process while Geyser is not running |
|
RedxAx and I were talking about this earlier, throughout the time after custom items support was added into Geyser, I've worked with multiple servers to convert packs over to Bedrock, but on live servers, Owners are easily able to add items & reload plugins such as nexo without requiring a server restart, however Geyser Mappings cannot be reloaded which leads to delays. Apart from that, I'm personally working on a converter, and as it has an api I plan to create a plugin that will convert certain plugin's packs for the users and setup the mappings and packs in place, and if mappings could be reloaded safely, that would remove the need for full server restarts and make this way more practical for production servers. As for the actual PR that's all upto him, he decided to do it, just replying to Redned's question on what this allows and how it'd be used. |
RedxAx
force-pushed
the
feat/reloadMapping
branch
from
f6b6e98 to
06da2fe
Compare
|
@onebeastchris done, mappings reload now live in the geyser reload cycle |
4 participants
I've made relevant mappings reloadable, by adding the ability for registries to be clear-able.
This can be done specifically with the
/geyser reloadmappingscommand, or implicitly with the normal reload command.This is very useful for an easier mcpacks development workflow, without a full restart, requested by a ResourcePack converter developer.