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.

Sign up for GitHub

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

Jump to bottom

Entity Helper Cleanup #2369

Merged
merged 2 commits into from
Sep 1, 2022
Merged

Entity Helper Cleanup #2369

merged 2 commits into from
Sep 1, 2022

Conversation

tal5
Copy link
Member

@tal5 tal5 commented Aug 31, 2022 •
edited

Changes

  • getFishHook was removed as the API it used is the same on all supported versions.
  • get/setAbsorption was removed as the API it used was the same on all supported versions.
  • setSleeping was removed in favor of the new EntityHelper#setPose.
  • setSneaking's NMS implementations were removed in favor of an implementation using EntityHelper#setPose.
  • getDiscoveredRecipes was removed in favor of HumanEntity#getDiscoveredRecipes.
  • getRawHoverText was removed as it wasn't used anywhere (as far as I could see) and all of it's implementations just threw an UnsupportedOperationException.
  • get/setBodyArrows were removed as they just called LivingEntity#get/setArrowsInBody with no NMS implementation.
  • get/setArrowPickupStatus was removed as the API it used is the same on all supported versions.
  • get/setItemFromTrident were removed as it wasn't used anywhere.
  • get/setSpeed were removed as all they did was get/set the speed attribute's base value, which is possible with API.
  • get/setShulkerPeek were removed in favor of Shulker#get/setPeek.
  • setGhastAttacking's 1.19 Implementation was removed in favor of Ghast#setCharging, and a TODO comment was added.
  • EntityPickupStatus property was changed to use AbstractArrow instead of Arrow.
  • getBoundingBox was removed in favor of Entity#getBoundingBox.
  • the com.denizenscript.denizen.nms.util.BoundingBox class was removed in favor of Bukkit's BoundingBox - all relevant code has been updated.
  • Changed the 1.16 EntityHelper implementation to directly call setPose instead of using reflection as it... doesn't seem to be necessary?
  • Made methods which are implemented on all versions abstract.
  • Let Intellij reorder/cleanup imports.

Additions

  • EnitityHelper#setPose sets an Entity's pose. added as an all-in-one replacement for other methods which did the same thing.

@mergu
Copy link
Contributor

mergu commented Aug 31, 2022

Often these NMS impls are made because the spigot-provided API wasn't enough. Have you validated every one of these changes through script, paying special attention also to the types of entities these work on (LivingEntity vs Entity vs ...)?

Also, have you validated this new API is also available on previous versions? Sometimes API is added but not backported via Spigot.

@tal5
Copy link
Member Author

tal5 commented Aug 31, 2022 •
edited

Often these NMS impls are made because the spigot-provided API wasn't enough. Have you validated every one of these changes through script, paying special attention also to the types of entities these work on (LivingEntity vs Entity vs ...)?

I tested all of these changes, obviously there could always be obscure edge-cases that the NMS impls were added for and I missed, but just to go over the few possibly more problematic changes:

  • getDiscoveredRecipes - HumanEntity#getDiscoveredRecipes reads from the same field in NMS, and Player extends HumanEntity.
  • get/setShulkerPeek - Shulker#get/setPeek uses the same NMS methods but divides / multiplies by 100 to make it a value from 0 to 1; I divided / multiplied it by 100 again to match the 0 to 100 format used by Denizen & NMS.
  • getBoundingBox - Entity#getBoundingBox uses the same NMS method.

Let me know if there are any other changes you're wondering about ^

Also, have you validated this new API is also available on previous versions? Sometimes API is added but not backported via Spigot.

I referenced https://helpch.at/docs/1.16.5 and tested on 1.16.5, so they should be all fine

mcmonkey4eva
mcmonkey4eva reviewed Aug 31, 2022
View reviewed changes
plugin/src/main/java/com/denizenscript/denizen/nms/interfaces/EntityHelper.java Outdated

public void setGhastAttacking(Entity entity, boolean attacking) {
public void setGhastAttacking(Entity entity, boolean attacking) { // TODO: 1.19 - Ghast#setCharging
Copy link
Member

Choose a reason for hiding this comment

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

If this has API in 1.19, remove the UnsupportedOperationException and remove the 1.19 impl, and make the interface impl use the API method (ie so old versions use NMS and new versions don't, and so we know more easily this is ready to remove once there are no implementing modules) (and still leave a // TODO: once minimum version is 1.19 or higher, remove from NMS comment as well of course)
(also for the record // TODO: 1.19 - ... would indicate an issue has been introduced by the 1.19 update and needs to be addressed ASAP)

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be fixed

mcmonkey4eva
mcmonkey4eva reviewed Sep 1, 2022
View reviewed changes
plugin/src/main/java/com/denizenscript/denizen/npc/traits/SleepingTrait.java
@@ -58,12 +57,7 @@ else if (npc.getEntity() instanceof Player) {
((Player) npc.getEntity()).sleep(bedLocation.clone(), true);
}
else {
if (NMSHandler.getVersion().isAtLeast(NMSVersion.v1_17)) {
Copy link
Member

Choose a reason for hiding this comment

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

As noted on discord, given that the only special case here is a very old version, better to not touch the 1.16 special case unless you're 100% confident this change is entirely correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted

mcmonkey4eva
mcmonkey4eva reviewed Sep 1, 2022
View reviewed changes
plugin/src/main/java/com/denizenscript/denizen/objects/EntityTag.java Outdated
@@ -3724,7 +3731,12 @@ else if (getBukkitEntity() instanceof Creeper) {
// Sets whether the ghast entity should show the attacking face.
// -->
if (mechanism.matches("ghast_attacking") && mechanism.requireBoolean()) {
NMSHandler.entityHelper.setGhastAttacking(getBukkitEntity(), mechanism.getValue().asBoolean());
if (NMSHandler.getVersion().isAtLeast(NMSVersion.v1_19)) { // TODO: 1.19
Copy link
Member

Choose a reason for hiding this comment

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

don't put that // TODO comment, the NMSVersion usage is the indicator

@mcmonkey4eva mcmonkey4eva merged commit 5f5fd36 into DenizenScript:dev Sep 1, 2022
@tal5 tal5 deleted the Entity_Helper_Cleanup branch September 1, 2022 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcmonkey4eva mcmonkey4eva mcmonkey4eva left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tal5 @mergu @mcmonkey4eva