Conversation
dktapps
left a comment
There was a problem hiding this comment.
Also I don't know what editor you're using but it's made an enormous mess of your indentation. Indents should use tabs.
|
Overall this is looking better since the last time I reviewed, but there's still some work left to do. Thanks for sticking with it 🙂 |
753ad00 to
f1a8cdd
Compare
dktapps
left a comment
There was a problem hiding this comment.
This looks mostly OK now, code-wise.
I think my main sticking points remaining are:
- There doesn't appear to be any way to disable the system, short of just setting the weather to clear for a very large duration. People aren't going to want it to rain in their lobbies.
- I don't like that we're constrained to clear/rainy/thunder.
- setRainLevel and setThunderLevel will not update the WeatherType, so that means plugins can make the
/weathercommand return wrong results anyway. - Instead of generally constraining it by the type, it would be better to have the
/weathercommand dynamically decide the weather type based on the rain and thunder amount (e.g. rain=0 + thunder=0 = clear, rain>0 + thunder=0 = rainy, thunder > 0 = thunder (whether or not it's raining)).
- setRainLevel and setThunderLevel will not update the WeatherType, so that means plugins can make the
a75eeea to
73aff4e
Compare
|
PocketMine-MP/src/network/mcpe/NetworkSession.php Line 1282 in e9a3bc6 PocketMine-MP/src/network/mcpe/handler/PreSpawnPacketHandler.php Lines 80 to 81 in e9a3bc6 |
|
It seems like snow isn’t acknowledged here either. |
|
Ok, I didn't want to come off as a jerk and say this so bluntly, but this has gone on long enough that the notification spam from here is really getting on my nerves. I'm on the verge of muting the PR. There are very basic errors in each commit that could be easily addressed by using the proper tools locally instead of spamming commits. Spamming commits chasing after CI failures is unproductive and it's also annoying for everyone getting notifications.
|
|
Got it, sorry about all the commit spam. I’ll run the checks on my end and won’t push small changes based on CI results anymore. Thanks for bringing this up! |
| } | ||
|
|
||
| protected function getInitialDragMultiplier() : float{ | ||
| return 0.02; |
|
|
||
| protected function initEntity(CompoundTag $nbt) : void{ | ||
| parent::initEntity($nbt); | ||
| $this->setNameTagVisible(false); |
|
I no longer know how this can be expanded... |
Adds a basic dynamic weather system with automatic transitions between clear weather, rain, and thunderstorms.
Rain, thunder, and lightning are handled server-side and synchronized for all players.
Related issues & PRs
Changes
Lightningweather entityWorld/weathercommandCompatibility
No breaking changes.
Worlds without weather support are unaffected.
Testing
Manual testing:
/weather rainand/weather thunderwork correctlyMinecraft_2025-10-28-21-49-18.mp4