-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: Add configurable maximum durability feature via config key "Item-Durability" #16
base: master
Are you sure you want to change the base?
feat: Add configurable maximum durability feature via config key "Item-Durability" #16
Conversation
src/main/java/com/cjcrafter/armormechanics/listeners/DurabilityListener.kt
Outdated
Show resolved
Hide resolved
src/main/java/com/cjcrafter/armormechanics/listeners/DurabilityListener.kt
Outdated
Show resolved
Hide resolved
… to fix issues on load
@CJCrafter did you see the latest changes & will you be able to make a review anytime soon? |
You still add a completely new placeholder system that isn't configurable when MechanicsCore has one baked in for ArmorMechanics. |
Thanks for answering, made changes. |
@CJCrafter did you see the latest changes & will you be able to make a review anytime soon? |
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.
At this point I am going to dive into the code myself and make changes to fit the project better (e.g. merge with WeaponMechanics, let WMP handle the advanced lore features, etc.). I have 2 comments to try and get resolved before I do that, which I left on this review.
src/main/java/com/cjcrafter/armormechanics/durability/DurabilityPlaceholderHandler.kt
Outdated
Show resolved
Hide resolved
totalDurability += getDurability(item) | ||
} | ||
} | ||
if (titles.distinct().size != 1) { |
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.
I see 3 or 4 data structures being created and I can't tell what this is supposed to be doing. I also side some side effects for non armor mechanics items...
e.g.
if (contents.size != 2) {
event.result = null
return
}
Why set the result to null? Why not just return? Seems like this might interfere with vanilla behavior or other plugins when we don't even know if there was an ArmorMechanics item involved.
e.g.
if (titles.distinct().size != 1)
This line and the loop above it uses 2 data structures to accomplish what a simple kotlin optional could handle.
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.
Understood the side effect part, will fix that.
This listener basically does:
For example we have an ArmorMechanics item "X"
When player tries to repair their X using another X in anvil plugin should return item with correct durability.
The reason I check distinct title size is for being sure they are not trying to fix X using Y (another ArmorMechanics item).
To get rid of data types I can probably just use one loop that is like:
for item in event.inventory.items
Continue if item is Air or Null
Add title of the item to titles Set (will check set size after loop is done)
Total durability += Durability of the item
Do you think this is what should be done?
I'm guessing these will resolve the last conversations right? @CJCrafter |
No description provided.