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.

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

exploded sf blocks now drop their sf item #3929

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

Conversation

iTwins
Copy link
Contributor

@iTwins iTwins commented Jul 30, 2023

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

Slimefun preview build

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

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
J3fftw1
J3fftw1 previously approved these changes Aug 5, 2023
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

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

Co-authored-by: JustAHuman-xD <[email protected]>
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
Contributor

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
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
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