feat: Add WeatherCommand for flexible weather control and persistence#1332
feat: Add WeatherCommand for flexible weather control and persistence#1332
Conversation
* Introduced WeatherCommand to allow setting weather conditions with optional persistence. * Implemented WeatherService to manage weather changes and persistence logic. * Replaced SunCommand, RainCommand, and ThunderCommand logic with WeatherService integration. * Enhanced weather-related notices to include persistence messages.
![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 introduces a WeatherService to centralize weather logic and adds a new /weather command that supports weather persistence by toggling the DO_WEATHER_CYCLE gamerule. Review feedback highlights a potential side effect where the gamerule is reset even when persistence isn't requested, suggests aligning /weather clear with vanilla behavior by setting the weather to sun, and notes localization issues caused by using enum names directly in messages.
| void setWeather(World world, WeatherType weatherType, boolean persistent) { | ||
| this.scheduler.run(() -> { | ||
| this.applyWeather(world, weatherType); | ||
| world.setGameRule(GameRule.DO_WEATHER_CYCLE, !persistent); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The current implementation of setWeather always updates the DO_WEATHER_CYCLE gamerule. If persistent is false, it explicitly sets it to true. This can unintentionally overwrite a global server setting where an administrator has disabled the weather cycle for other reasons (e.g., a static clear-sky world). It is safer to only modify the gamerule when persistence is explicitly requested, and let the /weather clear command handle re-enabling the cycle.
| private void clearWeather(Viewer viewer, World world) { | ||
| this.weatherService.clearPersistentWeather(world); | ||
|
|
||
| this.noticeService.create() | ||
| .viewer(viewer) | ||
| .placeholder("{WORLD}", world.getName()) | ||
| .notice(translation -> translation.timeAndWeather().weatherPersistenceDisabled()) | ||
| .send(); | ||
| } |
There was a problem hiding this comment.
The /weather clear command currently only re-enables the natural weather cycle but does not change the current weather state. In vanilla Minecraft, /weather clear is used to set the weather to sunny. This discrepancy might be confusing for players who expect the rain to stop immediately. Consider making this command also set the weather to SUN to align with vanilla expectations.
| this.noticeService.create() | ||
| .viewer(viewer) | ||
| .placeholder("{WORLD}", world.getName()) | ||
| .placeholder("{WEATHER}", weatherType.name().toLowerCase(Locale.ROOT)) |
There was a problem hiding this comment.
Using weatherType.name().toLowerCase(Locale.ROOT) for the {WEATHER} placeholder may lead to localization issues. In many languages (like Polish), the weather type needs to be a specific noun or adjective that agrees grammatically with the rest of the sentence (e.g., gender or case agreement). Hardcoding the enum name prevents proper translation of this part of the message.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c82544c4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void setWeather(World world, WeatherType weatherType, boolean persistent) { | ||
| this.scheduler.run(() -> { | ||
| this.applyWeather(world, weatherType); | ||
| world.setGameRule(GameRule.DO_WEATHER_CYCLE, !persistent); |
There was a problem hiding this comment.
WeatherService#setWeather now always writes DO_WEATHER_CYCLE to true when persistent is false, and all legacy /sun, /rain, and /thunder commands call this path with false. That introduces a regression where using those commands silently re-enables the weather cycle in worlds that were intentionally locked (by config, gamerule, or a prior /weather -p), changing global world behavior beyond just setting the current weather. Consider only changing DO_WEATHER_CYCLE when the user explicitly requests persistence changes (-p or clear).
Useful? React with 👍 / 👎.
1 participant
Added a new
/weathercommand for simple world weather control.Usage:
Behavior:
-pmakes the weather persistent by disabling the natural weather cycle.clearre-enables the natural weather cycle for the target world.