Skip to content
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

Armour #13

Merged
merged 19 commits into from
Dec 5, 2024
Merged

Armour #13

merged 19 commits into from
Dec 5, 2024

Conversation

dodonator
Copy link
Member

Implements an Armour class for things like clothing, hats etc. The Player has certain armour slots which can only hold one armour item at the same time. Armour items can be equiped just like weapons. The defense values of each armour piece is summed up in the Player.defense property.

@dodonator dodonator added the enhancement New feature or request label Oct 27, 2024
@dodonator dodonator requested review from inktrap and YtvwlD October 27, 2024 11:40
Copy link
Member

@YtvwlD YtvwlD left a comment

Choose a reason for hiding this comment

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

This seems cool!

Shouldn't the slots go to the character though?
You could probably also merge the two fixup commits to the one before, but that's not too important.

"""shows the players armour"""
for armour_type, armour_item in self.player.armour_slots.items():
if armour_item is None:
print(f"{armour_type}: None")
Copy link
Collaborator

@inktrap inktrap Nov 21, 2024

Choose a reason for hiding this comment

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

Can an armor type be None? If it can we should add a translated message for it, if not, it should be a warning through a logger. If it can't be none we could add a check #16 for it.

If we didn't care about this stuff until now it shouldn't block the PR but it doesn't hurt to keep good old issue #16 around

@@ -14,6 +14,7 @@ item-is-in-inventory = You already picked up this item.
pick-up-failed-message = You can't pick up this item.
go-failed-message = You can't go there.
equip-item-message = You equipped { INTER($item) }.
unequip-item-message = You took of { INTER(§item)} and took it in your inventory.
Copy link
Collaborator

Choose a reason for hiding this comment

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

So if you wear the armour or an item in general it takes up space in your inventory, right? So you can't over-equip. Otherwise it could not work to add it to the inventory.

Somehow I don't know if I am happy about this. Imagine a pretty efficient plate-like armour. It would be heavy and take up more space in your inventory, right? If you would carry a heavy weapon too, and you can have different types of armour, the inventory has to be huge or you can't carry much else. Or, and simpler is often better, an armour could be a fixed amount of slots regardless of effectiveness.

@inktrap
Copy link
Collaborator

inktrap commented Nov 21, 2024

So what is missing from this is the whole unequip flow and substracting defence points, right?

Looks fine :) good job :)

@dodonator dodonator merged commit 25148f4 into main Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants