Skip to content

Conversation

@crkellen
Copy link
Contributor

@crkellen crkellen commented Aug 21, 2017

This is a fix for #149.
The issue was the existing code for being hit by a boulder did not take into account Amulets of Life Saving, and therefore, was useless to protect against them. The new implementation checks to see if the Entity is a Player, and is wearing an Amulet of Life Saving, if so, act as if the Player survived the hit, by destroying the boulder.

Note: This code will have to be duplicated for the additional function added in #159.
Note: This code will have to be re-implemented for the newest changes in #159. I will do that after #159 is merged.

Copy link
Contributor

@lheckemann lheckemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I like the way you've done this, although it's symptomatic of a design issue with the original codebase as opposed to any decision on your behalf; killing entities all over the place leads to chaotic edge cases when anything about how death works changes, and the fact that this bug exists in the first place is also a symptom of this issue. It would be nice to fix death as a whole rather than just addressing the corner case. Overall I'm neutral on this — I suppose merging it would make sense since it fixes the issue but it's certainly not a good long-term solution.

@crkellen
Copy link
Contributor Author

crkellen commented Sep 24, 2017 via email

@crkellen
Copy link
Contributor Author

After reviewing, I can say that I agree with your statements. When looking for how life saving amulets worked, it surprised me how it was implemented. I could potentially look into refactoring how death is handled at a later point. For now, I think it's best to simply implement it like this, although it'll have to be re-implemented for the new changes in #159.

@WALLOFJUSTICE
Copy link
Collaborator

Did the changes in 41d55c7 to be merged, closing this PR

SheridanR pushed a commit that referenced this pull request Jul 30, 2023
fix off-by-one error in map gen
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants