Open deathcap opened 9 years ago
Patching the problem isn't exactly what should be in a PR. A more comprehensive fix, whatever it may be, would be much more suitable and desired.
Any suggestions on exactly how to more comprehensively fix this? I'm thinking of moving these code blocks:
if (currentlyRaining) {
setWeatherDuration(random.nextInt(12000) + 12000);
} else {
setWeatherDuration(random.nextInt(168000) + 12000);
}
into a new method, and calling it in the GlowWorld constructor.
If possible, set the ticks to zero THEN set the flags as false/true using the predetermined methods so long as extra events are not fired.
Added two new private methods scheduleStorm()
and scheduleThundering()
. These set the number of ticks until the next weather transition. Originally part of setStorm()
and setThundering()
, but those methods are public API and send events. The GlowWorld constructor now calls scheduleX to add the missing initialization for the rainingTicks
and thunderingTicks
member variables. This allows currentlyRaining
and currentlyThundering
to remain correctly initialized to false
.
Pull request looks good and is ready to be pulled. Assigning to @SpaceManiac for further review.
Currently all new worlds begin with both rain and thundering enabled. This PR changes this so new worlds begin with clear weather, no rain and no thundering.
Although currentlyRaining and currentlyThundering were initialized to false, since rainingTicks and thunderingTicks are initialized to 0, and are decremented to <= 0 on each tick, a weather transition occurs immediately, from false to true. Initializing the currentlyX variables to true flips this transition from false to true.
A more comprehensive fix might be a good idea, but would be more complex. I kept this PR as simple as possible to fix the observed behavior (new worlds annoyingly begin with stormy rain). May also want to investigate avoiding the initial weather transition (set rainingTicks and thunderingTicks to >0? but this value is random, calculated in setStorm() and setThundering(). Call setStorm(false) and setThundering(false) on newly-created worlds, to properly initialize the weather state? But this will still send the WeatherChangeEvents, may or may not be a problem), though I'd consider that improvement out of the scope of this PR; thoughts?