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

Allow sword of beheading drop piglin heads #3961

Merged
merged 4 commits into from
Dec 6, 2023
Merged

Allow sword of beheading drop piglin heads #3961

merged 4 commits into from
Dec 6, 2023

Conversation

ybw0014
Copy link
Contributor

@ybw0014 ybw0014 commented Aug 28, 2023 •
edited
Loading

Description

According to Minecraft wiki Head page, piglin head is available but there is no way to get it through Sword of Beheading. I think it should drop a piglin head when killing a piglin.

Proposed changes

  • Use enhanced switch.
  • Add piglin head as drop when killing piglins.

Related Issues (if applicable)

N/A

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.20.*).
  • 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.

@ybw0014 ybw0014 requested a review from a team as a code owner August 28, 2023 03:47
@github-actions
Copy link
Contributor

Pro Tip!
You can help us label your Pull Requests by using the following branch naming convention next time you create a pull request. ❤️

Branch naming convention Label
feature/** 🎈 Feature
fix/** ✨ Fix
chore/** 🧹 Chores
api/** 🔧 API
performance/** 💡 Performance Optimization
compatibility/** 🤝 Compatibility
If your changes do not fall into any of these categories, don't worry. You can just ignore this message in that case! 👀

@github-actions
Copy link
Contributor

github-actions bot commented Aug 28, 2023 •
edited
Loading

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: 0bd6820a

https://preview-builds.walshy.dev/download/Slimefun/3961/0bd6820a

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!

@JustAHuman-xD JustAHuman-xD added 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. 🎈 Feature This Pull Request adds a new feature. labels Aug 28, 2023
JustAHuman-xD
JustAHuman-xD reviewed Aug 28, 2023
View reviewed changes
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

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

May be out of scope for the PR but I wonder if theirs a better way to do that, currently it's very copy paste in the switch statement

...n/java/io/github/thebusybiscuit/slimefun4/implementation/items/weapons/SwordOfBeheading.java Outdated Show resolved Hide resolved
J3fftw1
J3fftw1 previously approved these changes Aug 28, 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.

i hate this code but I will approve it since this is how it has been done before.

JustAHuman-xD
JustAHuman-xD previously approved these changes Aug 30, 2023
View reviewed changes
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

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

LGTM and I agree with Jeff xd

Sfiguz7
Sfiguz7 requested changes Sep 6, 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.

Marking requested changes until the conversation above is resolved just in case it turns out to be something important. Feel free to ping me for reapproval if it is confirmed to be ininfluential

@ybw0014 ybw0014 dismissed stale reviews from JustAHuman-xD and J3fftw1 via 67fd518 September 7, 2023 04:39
JustAHuman-xD
JustAHuman-xD approved these changes Sep 9, 2023
View reviewed changes
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

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

LGTM, tested on 1.20.1 and all heads (apart from player cause I had no other players) dropped as they should

@variananora variananora added Build tested Used to indicate the PR preview build has been tested by one of the team and removed 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. labels Oct 5, 2023
JustAHuman-xD
JustAHuman-xD approved these changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

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

LGTM

J3fftw1
J3fftw1 approved these changes Nov 22, 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.

I dont like how this is done but LGTM

Sfiguz7
Sfiguz7 approved these changes Dec 6, 2023
View reviewed changes
Copy link
Member

@Sfiguz7 Sfiguz7 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Going off of my previous comment, LGTM but want a double check

@Sfiguz7 Sfiguz7 added 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. and removed Build tested Used to indicate the PR preview build has been tested by one of the team labels Dec 6, 2023
Sfiguz7

This comment was marked as resolved.

Sign in to view

Sfiguz7
Sfiguz7 approved these changes Dec 6, 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.

I swear I am not high, just saw it now has the version check. I need to shut up

@Sfiguz7 Sfiguz7 added Build tested Used to indicate the PR preview build has been tested by one of the team and removed 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. labels Dec 6, 2023
@Sfiguz7 Sfiguz7 merged commit 963349d into Slimefun:master Dec 6, 2023
17 checks passed
@ybw0014 ybw0014 deleted the feat/piglin-head branch February 14, 2024 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JustAHuman-xD JustAHuman-xD JustAHuman-xD approved these changes

@J3fftw1 J3fftw1 J3fftw1 approved these changes

@Sfiguz7 Sfiguz7 Sfiguz7 approved these changes

@Sefiraat Sefiraat Awaiting requested review from Sefiraat

Assignees

@Sfiguz7 Sfiguz7

@J3fftw1 J3fftw1

@JustAHuman-xD JustAHuman-xD

Labels
Build tested Used to indicate the PR preview build has been tested by one of the team 🎈 Feature This Pull Request adds a new feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ybw0014 @Sefiraat @Sfiguz7 @J3fftw1 @JustAHuman-xD @variananora