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

Add events to Lumber Axe & Woodcutter Android #4228

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add events to Lumber Axe & Woodcutter Android #4228

wants to merge 3 commits into from

Conversation

JustAHuman-xD
Copy link
Contributor

Description

Due to the lack of events with the lumber axe & woodcutter androids there are currently problems with the "SurperiorSkyblock2" plugin. (See issue referenced)

Proposed changes

  • Added AndroidBlockBreakEvent
    • Meant to be a generic event whenever any android breaks a block
    • Based on the old AndroidMineEvent
  • Rewrote AndroidMineEvent
    • Now extends AndroidBlockBreakEvent and all duplicate functionality has been removed
  • Added AndroidWoodcutEvent
    • Not sold on the name but is what it is atm
    • Called whenever a Woodcutter android tries to break a log
  • Added a BlockStorage check to the Woodcutter Android, I'm concerned that there wasn't one already lol
  • Changed the Lumber Axe to call a BlockBreakEvent for every log it breaks & respect the event's dropItems field.

Related Issues (if applicable)

BG-Software-LLC/SuperiorSkyblock2#2198

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.

@JustAHuman-xD JustAHuman-xD marked this pull request as ready for review August 30, 2024 16:12
@JustAHuman-xD JustAHuman-xD requested a review from a team as a code owner August 30, 2024 16:12
@github-actions github-actions bot added the ✨ Fix This Pull Request fixes an issue. label Aug 30, 2024
@github-actions GitHub Actions
Copy link
Contributor

Your Pull Request was automatically labelled as: "✨ Fix"
Thank you for contributing to this project! ❤️

@JustAHuman-xD JustAHuman-xD added the 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. label Aug 30, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Aug 30, 2024 •
edited
Loading

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: 82fbbbf

https://preview-builds.walshy.dev/download/Slimefun/4228/82fbbbf4

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!

Sfiguz7
Sfiguz7 approved these changes Aug 31, 2024
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 approved these changes Sep 6, 2024
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.

Seems good to me, if we could still get it tested real quick I'd be happier though

@Sfiguz7
Copy link
Member

Sfiguz7 commented Sep 6, 2024

Ok wow I meant to edit my comment but sure

@JustAHuman-xD
Copy link
Contributor Author

I did test it a bit on my own and it worked as expected but another test would be good I agree

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

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
✨ Fix This Pull Request fixes an issue. 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JustAHuman-xD @Sfiguz7