Refactor environment effect lookup

[Veritania]

Looking for feedback. Trying to tackle Issue #251

I'm not a fan of just using raw strings to define day/night/good/bad/neutral, but went with it for now due to WeatherInfo.cs and GameTimeInfo.cs returning booleans for these same values.

[MikeJeffers]

Hey this is great! Happy someone picked up one of the refactoring issues.
Anything to clean up all the nested conditionals is a win. I think your strategy to assign strings to each state is a fine approach and would be happy to review and merge as-is.

If you are feeling ambitious, maybe those functions return a DayState or WeatherState enum so you don't have string literals floating about, and the methods might not be IsDay or IsNight but GetDayState allowing us to have more nuanced segments of day or weather.
But that can be a separate PR for later, totally up to you!

Thank you for the PR!!

[Veritania]

Hi, thank you for taking a look at it! I'm happily surprised about the positive enthusiasm :)

I've tried to add the enums DayState and WeatherState as you recommended, and the EventHandler is now using those to determine the next weather effect.

[Veritania]

This can happen when both

WeatherInfo(Between GOOD_WEATHER = 100 and BAD_WEATHER = 200)
and
GameTimeInfo (Between NIGHT_END = 100 and DAY_START = 200)

hits some of their undefined gaps at the same time, where it's neither Day or Night / Good or Bad. So now they would default to 'NEUTRAL'.

Old logic just did nothing in that case, but here we would try to lookup a value in the _weatherDict which does not exist (neutral, neutral).

So to avoid that lookup, I just return;
But I'm not sure if we ever have a scenario where both of them hits these gaps at the same time.

[MikeJeffers]

Yes, both weather and daytime can have a neutral state and not have any zone effect.
So its a bit weird, but you do need to allow it to look up a null-entry so that it will cancel the current zone-effect.
The isSameEffect logic handles this stuff.
So in this case, you shouldn't actually return, but let the ZoneEffect nextEffect = null; carry through and cancel the current effect.
tl;dr don't return, and add an entry for null in your dictionary - should achieve the desired outcome

I'll pull the code and test it out locally soon, but I do remember this is a state that the effect can/should be in when both are 'neutral' states.

[Veritania]

Thank you for the insight!

I have now flipped the logic of the if statement, so that if both are in the neutral state we don't touch the initial ZoneEffect nextEffect = null and pass it to the isSameEffect unchanged. It should have the same result as you described.

If you instead prefer that there is a null entry in the dictionary, I'd have to look into how to solve that.

[MikeJeffers]

I think you can just add another entry with {(neutral, neutral): null} and let it look up and get the null value from the dict.
That way you avoid that one remaining if statement wrapping the lookup - that way the mapping is all contained in the dictionary.

[Veritania]

I've pushed #a35a0f2 which tries to achieve that.
There is no longer any if statement, and the dictionary contains an entry for neutral+neutral which returns null.

The alternative to having dynamic instead of EffectType as the expected values in the dictionary, was to mark it with EffectType? as a nullable. But that had some ripple-effects throughout the class, affecting the other dictionary and methods.

So I think this looks a little cleaner in comparison.

[MikeJeffers]

Actually, hang on a sec. I just realized you are creating a dictionary of EffectTypes and not the ZoneEffect instances - which means we can simplify this into one dictionary! (woo!) No dynamic type, and you have a nullable class instance.

You see the collection that is made here? https://github.com/OpenPerpetuum/PerpetuumServer/blob/Development/src/Perpetuum/Services/EventServices/EventProcessors/EnvironmentalEffectHandler.cs#L37-L45
Instead of having that key on EffectType have it key on your Tuple of Time/weather states!
That way you save creating another static dictionary, avoid using dynamic, and skip a lookup (O(1), but still more to maintain).
Then GetEffect would have a signature like: ZoneEffect GetEffect(GameTimeState, WeatherState) or the tuple, either way. (maybe cleaner to create the tuple in the helper method?)

Sorry I did not look closely enough to realize you were replacing that dictionary, and it works and cleans up a lot of code, but if you want to this extra tweak will really make this a killer PR!
This is the danger of not pulling the code first before reviewing 🤦

Let me know if this makes sense or if you have any questions.

[Veritania]

Thank you for the feedback. I think I understand what you mean.
I've pushed #3976274 to try and meet those requirements.

I'm at fault here, for not having better understanding of the code base I'm making changes to.
But I'm happy that I can receive guidance from you, and please be critical. I want to continue learning from this, and hopefully contribute value in the process.

[MikeJeffers]

Great to hear! I want open source projects like this to be opportunities for people to learn and work on things they otherwise might not be able to build themselves. But also don't want to make it too hard to contribute either. Either way I'll work with you to make sure you get your code in the repo.

The changes look good! I'll get you a review and maybe a merge later this evening.
And don't worry, we are still learning the codebase ourselves too ;)

[MikeJeffers]

one line change to consider

[MikeJeffers]

Any reason to not simply declare and initialize nextEffect here? (why have ln61?)

[MikeJeffers]

var nextEffect = GetEffect(_gameTime.GetDayState(), _weatherState.getWeatherState()); basically.
^ var can be used because its no longer initialized with null.

Style nit: Also there is some trailing whitespace on this line. (I finally pulled the code!)

[MikeJeffers]

var nextEffect = GetEffect(_gameTime.GetDayState(), _weatherState.getWeatherState()); basically.
^ var can be used because its no longer initialized with null.

Style nit: Also there is some trailing whitespace on this line. (I finally pulled the code!)

[MikeJeffers]

Tested! Working as intended.
Feels so good to see this refactored... OnStateChange is so clean now :)

[Veritania]

Latest commit should resolve your requests :) Thank you for the review

[MikeJeffers]

Amazing work @Veritania !
Congratulations on your first contribution 🎉