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

Ancient Pedestal Fixes #3784

Merged
merged 12 commits into from
Jul 12, 2023
Merged

Ancient Pedestal Fixes #3784

merged 12 commits into from
Jul 12, 2023

Conversation

JustAHuman-xD
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD commented Feb 26, 2023 •
edited
Loading

Description

To fix the various problems people can encounter with the ancient pedestal, this is my re-write of #3637

Proposed changes

  • Added ArmorStandUtils.java
  • Added ArmorStandUtils#createArmorStand
  • HologramProjector now uses ArmorStandUtils#createArmorStand
  • Ancient Pedestal now implements NotHopperable
  • Ancient Pedestal now uses ArmorStandUtils#createArmorStand to create an invisible armorstand, and sets the item as its passenger to prevent it from being moved by things like fluids and falling blocks
  • Marked the item for an ancient pedestal be Invulnerable & UnlimitedLifeTime (To fix items despawning and making players loose their items)

Related Issues (if applicable)

Resolves #2940
Resolves #3147
Resolves #3444
Resolves #3507

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
  • I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.19.*).
  • I followed the existing code standards and didn't mess up the formatting.
  • I did my best to add documentation to any public classes or methods I added.
  • I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
  • I added sufficient Unit Tests to cover my code.

@JustAHuman-xD JustAHuman-xD requested a review from a team as a code owner February 26, 2023 14:56
@TheBusyBiscuit TheBusyBiscuit self-assigned this Mar 2, 2023
@TheBusyBiscuit TheBusyBiscuit added the ✨ Fix This Pull Request fixes an issue. label Mar 2, 2023
Sefiraat
Sefiraat previously approved these changes Mar 4, 2023
View reviewed changes
Copy link
Member

@Sefiraat Sefiraat left a comment

Choose a reason for hiding this comment

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

Feel like we can/should expand the util a little bit that's out of scope for the PR and what you have here looks good to me!

@JustAHuman-xD
Copy link
Contributor Author

Going to make a commit to fix the import order but other than that any chance this could be reviewed?

@JustAHuman-xD JustAHuman-xD requested review from Sefiraat and a team June 11, 2023 16:45
@JustAHuman-xD
Fix Slimefun#3507
b4d2020
@JustAHuman-xD
Remove Initial Flicker
060b59d 
By Setting the Entity Tags in this lambda I am setting them before the entity is spawned. It removes the currently flicker of an armorstand when you place an item for the first time. Its a small thing but makes it that much more immersive & polished.
@JustAHuman-xD
Copy link
Contributor Author

Any chance this could get reviewed soon?

svr333
svr333 previously approved these changes Jun 24, 2023
View reviewed changes
Copy link
Member

@svr333 svr333 left a comment

Choose a reason for hiding this comment

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

looks good to me, didnt see anything

WalshyDev
WalshyDev reviewed Jun 24, 2023
View reviewed changes
Copy link
Member

@WalshyDev WalshyDev left a comment

Choose a reason for hiding this comment

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

Looks good, just a few small things

src/main/java/io/github/thebusybiscuit/slimefun4/utils/ArmorStandUtils.java Show resolved Hide resolved
src/main/java/io/github/thebusybiscuit/slimefun4/utils/ArmorStandUtils.java Outdated Show resolved Hide resolved
src/main/java/io/github/thebusybiscuit/slimefun4/utils/ArmorStandUtils.java Outdated Show resolved Hide resolved
@JustAHuman-xD @WalshyDev
Suggested Change
7b4f533 
Co-authored-by: Daniel Walsh <[email protected]>
@github-actions
Copy link
Contributor

github-actions bot commented Jun 24, 2023 •
edited
Loading

Slimefun preview build

A Slimefun preview build is available for testing!

https://preview-builds.walshy.dev/download/Slimefun/3784/5400670196

Note: This is not a supported build and is only here for the purposes of testing.
Do not run this on a live server and do not report bugs anywhere but this PR!

WalshyDev
WalshyDev previously approved these changes Jun 24, 2023
View reviewed changes
@WalshyDev WalshyDev self-assigned this Jun 24, 2023
@variananora
Copy link
Member

variananora commented Jul 6, 2023 •
edited
Loading

Just tested this based on commit 1f407b7 (#3784)

What works:

  • Placing item on ancient pedestal will not despawn after the item despawn rate exceeds.
  • Placing item on ancient pedestal and taking it back will not change the name (was a problem with potions).
  • Placing item on ancient pedestal and trying to move it with gravity block, block placer, water, or lava will not move the item.
  • Ancient altar functionality is still working as intended.
  • Breaking ancient pedestal with items will drop both the item and ancient pedestal.
  • Ancient pedestal now can't be filled items with hopper (the vanilla mechanics).
  • The floating item on ancinet pedestal can't be sucked by a hopper.
  • Hologram projector is still working as intended

@variananora variananora added the Build tested Used to indicate the PR preview build has been tested by one of the team label Jul 6, 2023
@JustAHuman-xD
Copy link
Contributor Author

Just tested this based on commit 1f407b7 (#3784)

Did you happen to test that the hologram projector still worked as intended? As it's code was changed.

@variananora
Copy link
Member

variananora commented Jul 7, 2023 •
edited
Loading

Did you happen to test that the hologram projector still worked as intended? As it's code was changed.

Ah I didn't see that, will update the test later this noon

Update: It works fine

@JustAHuman-xD
Copy link
Contributor Author

Just tested old ancient pedestals having items on and stopping the server, switching to test build and was able to properly pick them and generate the armorstands upon using it again, unless anyone has anything else to test it should be good 👍

J3fftw1
J3fftw1 approved these changes Jul 9, 2023
View reviewed changes
Copy link
Contributor

@J3fftw1 J3fftw1 left a comment

Choose a reason for hiding this comment

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

LGTM

Sfiguz7
Sfiguz7 approved these changes Jul 12, 2023
View reviewed changes
Copy link
Member

@Sfiguz7 Sfiguz7 left a comment

Choose a reason for hiding this comment

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

LGTM

@Sfiguz7 Sfiguz7 merged commit f2fb911 into Slimefun:master Jul 12, 2023
8 checks passed
@JunederZ JunederZ mentioned this pull request Jul 27, 2023
Closed
4 tasks
@JustAHuman-xD JustAHuman-xD deleted the fix/ancient-pedestal branch September 9, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sfiguz7 Sfiguz7 Sfiguz7 approved these changes

@J3fftw1 J3fftw1 J3fftw1 approved these changes

@WalshyDev WalshyDev WalshyDev left review comments

@svr333 svr333 svr333 left review comments

@Sefiraat Sefiraat Awaiting requested review from Sefiraat

Assignees

@TheBusyBiscuit TheBusyBiscuit

@WalshyDev WalshyDev

@Sefiraat Sefiraat

@Sfiguz7 Sfiguz7

@J3fftw1 J3fftw1

Labels
Build tested Used to indicate the PR preview build has been tested by one of the team ✨ Fix This Pull Request fixes an issue.
Projects
None yet
Milestone
No milestone
8 participants
@JustAHuman-xD @variananora @WalshyDev @Sefiraat @svr333 @Sfiguz7 @J3fftw1 @TheBusyBiscuit