Skip to content

Dungeon: Fix wrong item removal by using equals-based matching#2771

Open
Shruti39 wants to merge 7 commits intoDungeon-CampusMinden:masterfrom
Shruti39:shruti/inventory-removeone-fix
Open

Dungeon: Fix wrong item removal by using equals-based matching#2771
Shruti39 wants to merge 7 commits intoDungeon-CampusMinden:masterfrom
Shruti39:shruti/inventory-removeone-fix

Conversation

@Shruti39
Copy link

Fixes #2698

This PR fixes a bug where InventoryComponent.removeOne removed the wrong
item when multiple items of the same class but different attributes
(e.g. health potions with different heal amounts) were present.

Changes:

  • Updated removeOne(Item) to prefer exact instance matches and fall back
    to equals-based matching
  • Overrode equals/hashCode in ItemPotionHealth to define semantic equality
    based on heal_amount

Other InventoryComponent methods already rely on equals(Item) and
therefore benefit automatically from the corrected equality semantics.
Class-based query methods were intentionally left unchanged.

…ased removal logic with instance-first and equals-based item matching. This ensures health potions with different heal amounts are removed correctly. Fixes Dungeon-CampusMinden#2698.
@Flamtky Flamtky changed the title Fix wrong item removal by using equals-based matching Dungeon: Fix wrong item removal by using equals-based matching Jan 15, 2026
@Flamtky Flamtky requested a review from AMatutat January 15, 2026 16:52
@AMatutat AMatutat requested a review from Flamtky January 19, 2026 07:21
Comment on lines 317 to 329
@@ -330,28 +328,32 @@ public Optional<Item> get(int index) {
* inventory.
*/
Copy link
Member

Choose a reason for hiding this comment

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

This Javadoc now needs to be updated.

Co-authored-by: Niklas Schumann <68606032+Flamtky@users.noreply.github.com>
@AMatutat
Copy link
Contributor

@Shruti39 can you give me an update?

If you resolved @Flamtky feedback, you have to re-request an new review, so he sees it.

@Shruti39
Copy link
Author

Shruti39 commented Feb 1, 2026

@Shruti39 can you give me an update?

If you resolved @Flamtky feedback, you have to re-request an new review, so he sees it.

Set for start items no longer works when duplicate items (same class + attributes) are intended, e.g. multiple arrows.

I agree that start items should be represented as an ordered collection that allows duplicates (e.g. Item[] or List), since the amount is known upfront.

I can take this issue and prepare a PR that:

  • replaces Set with Item[] (or List) for start items in CharacterClass
  • updates the constructor and usages accordingly

Let me know if Item[] is preferred over List.

Co-authored-by: Niklas Schumann <68606032+Flamtky@users.noreply.github.com>
@Flamtky
Copy link
Member

Flamtky commented Feb 6, 2026

I agree that start items should be represented as an ordered collection that allows duplicates (e.g. Item[] or List), since the amount is known upfront.

I can take this issue and prepare a PR that:

  • replaces Set with Item[] (or List) for start items in CharacterClass
  • updates the constructor and usages accordingly

Let me know if Item[] is preferred over List.

We will use lists. I have already opened a PR for this change => #2779. This will allow us to focus on resolving issue #2698 in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

removeOne removes wrong Item

3 participants