Conversation
… target selection
…ronous time updates
There was a problem hiding this comment.
Pull request overview
This PR aims to resolve issue #26 by addressing broken Minecraft command behaviors (notably selector handling in /tp and time changes under Folia scheduling).
Changes:
- Dispatches
/timeworld-time mutations viaplugin.globalRegionDispatcher. - Updates
/tpdestination argument from “one player” to “one entity” to support selectors like@e[...]. - Removes the “TODO: add NBT Support” comment from
/summon.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/main/kotlin/dev/slne/surf/essentials/command/minecraft/TimeCommand.kt |
Routes some time mutations through Folia’s globalRegionDispatcher coroutine context. |
src/main/kotlin/dev/slne/surf/essentials/command/minecraft/TeleportCommand.kt |
Allows teleport target to be any entity instead of only a player. |
src/main/kotlin/dev/slne/surf/essentials/command/minecraft/SummonCommand.kt |
Removes NBT support TODO comment (no functional NBT change). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Bukkit.getWorlds().forEach { | ||
| it.fullTime = time.toLong() | ||
| plugin.launch(plugin.globalRegionDispatcher) { | ||
| it.fullTime = time.toLong() | ||
| } | ||
| } |
There was a problem hiding this comment.
Bukkit.getWorlds().forEach { plugin.launch(plugin.globalRegionDispatcher) { ... } } launches a separate coroutine per world, and the command immediately sends the success message without waiting for any of them to run. Consider launching a single coroutine on globalRegionDispatcher and updating all worlds inside it (and optionally sending the confirmation after the updates), to reduce coroutine overhead and keep the command feedback consistent with when the state is actually applied.
| entitySelectorArgumentManyPlayers("players") { | ||
| withPermission(EssentialsPermissionRegistry.TELEPORT_COMMAND_OTHERS) | ||
| entitySelectorArgumentOnePlayer("target") { | ||
| entitySelectorArgumentOneEntity("target") { | ||
| anyExecutor { executor, args -> | ||
| val players: Collection<Player> by args | ||
| val target: Player by args | ||
| val target: Entity by args |
There was a problem hiding this comment.
This updates the destination selector to entitySelectorArgumentOneEntity, but the multi-target variant still uses entitySelectorArgumentManyPlayers("players"), which prevents selectors like @e[...] in the first argument position (vanilla /tp allows teleporting non-player entities too). If issue #26 is about @e[...] selectors generally, consider switching the first argument to a many-entities selector and teleporting those entities accordingly.
| } | ||
| } | ||
| }//TODO: add NBT Support | ||
| } |
There was a problem hiding this comment.
The PR description says it solves issue #26, which also mentions broken /summon NBT usage. This change only removes the TODO comment, but the command implementation still doesn't accept/pass through NBT data, so /summon ... { ... } will remain unsupported/broken if this command overrides vanilla. Either add an NBT argument/pass-through, or update the PR scope/description to match what’s actually fixed.
| Bukkit.getWorlds().forEach { world -> | ||
| val current = world.fullTime % 24000 | ||
| val target = namedTime.ticks % 24000 | ||
|
|
||
| val diff = if (target >= current) { | ||
| target - current | ||
| } else { | ||
| (24000 - current) + target | ||
| } | ||
|
|
||
| world.fullTime += diff | ||
| plugin.launch(plugin.globalRegionDispatcher) { | ||
| world.fullTime += diff | ||
| } |
There was a problem hiding this comment.
In the namedTime branch, current = world.fullTime % 24000 and diff are computed outside the launched coroutine, but the actual world.fullTime += diff happens later on globalRegionDispatcher. Because the update is delayed/asynchronous, fullTime may change between computing diff and applying it, causing the world to end up at the wrong time. Compute current/diff inside the dispatched block (and read/write fullTime on the same dispatcher) so the calculation is consistent with the applied value.
src/main/kotlin/dev/slne/surf/essentials/command/minecraft/TimeCommand.kt
Show resolved
Hide resolved
1 participant
solves #26