-
Notifications
You must be signed in to change notification settings - Fork 869
Extract common code from bow and crossbow #5351
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
base: 1.20.1
Are you sure you want to change the base?
Conversation
|
This PR is the first step toward allowing mob to use TConstruct bows and arrows. They can call |
src/main/java/slimeknights/tconstruct/library/tools/item/ranged/LauncherUserInfo.java
Outdated
Show resolved
Hide resolved
src/main/java/slimeknights/tconstruct/library/tools/item/ranged/ModifiableLauncherItem.java
Show resolved
Hide resolved
| if (user instanceof Player player) { | ||
| player.getInventory().removeItem(standardAmmo); | ||
| } |
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 want to discuss this more; how is a non-player expected to remove the ammo from their inventory? Or are we just assuming they don't care about cleaning up empty stacks?
I know most mobs do infinite ammo, but imagine a dispenser like shooter.
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 would say the mob needs to be able to clean up empty stacks themselves. What harm will it do to not remove empty stacks? They are empty and isEmpty() returns true
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.
Mostly just memory waste, which is why vanilla typically does it.
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 don't think there is a realistic method to clear mob inventory, as each mob would have different implementations. Sounds like it doesn't matter that much.
Though if you think this part should be part of the API, then maybe LauncherUserInfo would be a place to add it.
src/main/java/slimeknights/tconstruct/library/modifiers/hook/ranged/BowAmmoModifierHook.java
Outdated
Show resolved
Hide resolved
# Conflicts: # src/main/java/slimeknights/tconstruct/library/tools/item/ranged/ModifiableLauncherItem.java
src/main/java/slimeknights/tconstruct/library/tools/item/ranged/ModifiableLauncherItem.java
Outdated
Show resolved
Hide resolved
|
Any remaining items to discuss? |
|
I need to do a more in depth code review when I get a chance. I might have something after that. |
|
Thanks |
Simplifies an upcoming change. Related issue: #5351
BowAmmoModifierHook: Change input parameter type fromPlayertoLivingEntityRestructure arrow shoot logic: