New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for Thirst related Alive/Death handling #5
Conversation
It looks good to me! Thanks for the corrections and formatting changes as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the recent work on exposing more internal logic via events in DynamicCities, the question above below came to my mind....
What do you think about this?
thirst.lastCalculatedWater = ThirstUtils.getThirstForEntity(entity); | ||
public void onPlayerRespawn(OnPlayerRespawnedEvent event, EntityRef player, | ||
ThirstComponent thirst) { | ||
logger.info("mark"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray logging statement feels lonely 😢
resetThirst(player, thirst); | ||
} | ||
|
||
private void resetThirst(EntityRef player, ThirstComponent thirst) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to expose the resetThirst
functionality as event + handler? That way, another module could modify the behavior in case a player respawns after death. For instance, start only with half the thirst meter in general, and only respawn with full thirst meter if you have some item equipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems sensible, although it may get lost on this closed PR. Maybe better to submit as an enhancement request issue :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That'd seem interesting @skaldarnar @Cervator. I'd suggest submitting an issue with this idea.
Fix for MovingBlocks/Terasology#3003