Skip to content

Wrong requirement when trying to equip and level too low#512

Open
hydrozoa-yt wants to merge 1 commit intoluna-rs:masterfrom
hydrozoa-yt:fix-equipmentDefinitionFileparser
Open

Wrong requirement when trying to equip and level too low#512
hydrozoa-yt wants to merge 1 commit intoluna-rs:masterfrom
hydrozoa-yt:fix-equipmentDefinitionFileparser

Conversation

@hydrozoa-yt
Copy link
Copy Markdown
Contributor

Proposed changes

Currently, when trying to equip anything and the level is too low, it will say "You need an Attack level of x to equip this". This happens for example for mirror shield and rune boots, even though they don't have an attack requirement.

This happens due to the way EquipmentDefinition is loaded. When loaded using Gson#fromJson the objects are instantiated using reflection, and so the constructor of EquipmentDefinition.Requirement is never ran. This causes the level field to never be instantiated, as this happens in the constructor. This field determines the behavior of the Requirement, so this bug is critical for equipment working properly.

This PR fixes the bug by removing final from the level field, allowing it to be instantiated in a different method, and forcibly running this method after loading. I've also added it to the unused constructor, should the way Requirements are loaded change in the future.

Pull Request type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)

Further comments

A different fix to this could be to use the streaming api in gson to load the json file. This would be a bit more verbose than the Gson#fromJson solution, but would allow "normal" instantiation of the objects, and keep the level field final.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant