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

exploded sf blocks now drop their sf item #3929

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

exploded sf blocks now drop their sf item #3929

wants to merge 4 commits into from

Conversation

iTwins
Copy link
Contributor

@iTwins iTwins commented Jul 30, 2023 •
edited

Description

Slimefun blocks didn't drop when exploded because the drops argument in BlockBreakHandler::onExplode did nothing.

Proposed changes

Pass the SlimefunItem to the handleEplosion method in ExplosionsListener.java and drop the item there.

Other changes:
Removed unused import.

I didn't change the method signature for BlockBreakHandler::onExplode since it's a public method and that seemed like a bad idea to change.

EDIT: Changed approach completely. Now added drops logic to SimpleBlockBreakHandler::onExplode.

Related Issues (if applicable)

Resolves #3601
Also resolves Slimefun blocks not dropping from explosions, but somehow there was no issue open

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.

@iTwins iTwins requested a review from a team as a code owner July 30, 2023 10:49
@github-actions github-actions bot added the ✨ Fix This Pull Request fixes an issue. label Jul 30, 2023
@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

github-actions bot commented Jul 30, 2023 •
edited

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: a18957a

https://preview-builds.walshy.dev/download/Slimefun/3929/a18957ab

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!

J3fftw1
J3fftw1 previously approved these changes Jul 30, 2023
View reviewed changes
J3fftw1
J3fftw1 previously approved these changes Aug 5, 2023
View reviewed changes
Copy link
Member

@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

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

Code Style guidelines thing, otherwise looks good to me, can we get testing on this, feels like a big change, even if it's small code wise

...java/io/github/thebusybiscuit/slimefun4/implementation/handlers/SimpleBlockBreakHandler.java Outdated Show resolved Hide resolved
@iTwins @JustAHuman-xD
if statement brackets
e294761 
Co-authored-by: JustAHuman-xD <65748158+JustAHuman-xD@users.noreply.github.com>
JustAHuman-xD
JustAHuman-xD approved these changes Sep 6, 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

@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 Sep 6, 2023
@Boomer-1
Copy link

i built a wall of 5x5 slimefun items. I hit the center with the upgraded explosive pickaxe from fluffy machines and only the center block dropped. however using the core slimefun explosive pickaxe (3x3) everything dropped as expected and stayed slimefun items. creeper explosions and tnt did not drop any slimefun blocks, nor were the blocks damaged. that doesn't seem like that should be the case. if a vanilla block explodes, so should the slimefun block

@iTwins
Copy link
Contributor Author

iTwins commented Nov 23, 2023

I just tested this again, and for me the blocks were broken and dropped for the explosive pickaxe, tnt and creepers.
The Fluffy explosive pickaxe code says it shouldn't remove Slimefun blocks, so I guess that's working as intended.

SlimefunItem sfItem = BlockStorage.check(b);

// Don't break SF blocks
if (sfItem != null) {
    return;
}

@Boomer-1
Copy link

in doing additional testing, some slimefun items broke, where others do not. core slimefun that broke were regulators and cargo and core machines. addon blocks that i tested didnt break. so far i can't get any addon blocks to drop.

@iTwins
Copy link
Contributor Author

iTwins commented Nov 23, 2023

The way it's implemented now allows devs to choose what behaviour they want. This pr changes SimpleBlockBreakHandler which is used by core sf. If we wanted the blocks to drop regardless of how BlockBreakHandler::onExplode is implemented we could drop the blocks in the ExplosionsListener instead.

@Boomer-1
Copy link

whichever way the team wants to go. just providing testing results. thanks itwins

@iTwins
Copy link
Contributor Author

iTwins commented Nov 23, 2023

And thank you a lot for testing 😁 !!

@J3fftw1 J3fftw1 closed this Dec 31, 2023
@J3fftw1 J3fftw1 reopened this Dec 31, 2023
@J3fftw1
Copy link
Member

J3fftw1 commented Dec 31, 2023

it doesnt seem to work for me.
However i think this pr handles it already
#4062
alessio just needs to adress those comments if thats merged we need to rebase this pr and see if its even still needed and/or if it fixes it

@J3fftw1 J3fftw1 added the ⚠BLOCKED This pull request is currently blocked from merging for technical reasons. label Dec 31, 2023
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 left review comments

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

Assignees
No one assigned
Labels
⚠BLOCKED This pull request is currently blocked from merging for technical reasons. ✨ 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.

Enabling call-explosion-event will not drop Slimefun blocks when breaking it using explosive tools.
4 participants
@iTwins @Boomer-1 @J3fftw1 @JustAHuman-xD