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

Fix sensitive blocks attached to sf blocks not dropping (1.19+) #3966

Merged
merged 3 commits into from
Dec 8, 2023

Conversation

iTwins
Copy link
Contributor

@iTwins iTwins commented Sep 7, 2023

Description

When you would break a Slimefun block any sensitive blocks attached to it would be destroyed, but not drop their item.
This fix only works for 1.19+, but that accounts for 4/5 of all Slimefun servers. Feel free to suggest a 1.16-1.18 version.

Proposed changes

Recursively check all attached sensitive blocks and drop them if they should be, while respecting max-chained-neighbor-updates in server.properties.

Related Issues (if applicable)

Resolves #3182
Resolves #3831

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 September 7, 2023 03:42
@github-actions github-actions bot added the ✨ Fix This Pull Request fixes an issue. label Sep 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

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

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

Slimefun preview build

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

https://preview-builds.walshy.dev/download/Slimefun/3966/f3861ebd

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!

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.

Won’t this create a ghost block if the sensitive block is a Slimefun block?

@iTwins
Copy link
Contributor Author

iTwins commented Sep 7, 2023

If it does now then it already did before this change. The only extra thing I do now is drop the items of sensitive blocks that get destroyed. Also the only 2 sensitive blocks in Slimefun are the elevator plate and teleportation matrix activator and those get handled by checkForSensitiveBlockAbove before my method runs.

This new method could serve as the more inclusive replacement for checkForSensitiveBlockAbove if I check blockstorage for each of the destroyed blocks, but before that's possible the 1.16-1.18 fix has to be implemented.

if (Slimefun.getMinecraftVersion().isAtLeast(MinecraftVersion.MINECRAFT_1_19)) {
return blockData.isSupported(block);
} else {
// TODO: Make 1.16-1.18 version. BlockData::isSupported is 1.19+.
Copy link
Contributor

Choose a reason for hiding this comment

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

is this todo for this PR or later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't planning on making that in this pr and I doubt anyone else will, so worst case scenario this is a fix for only 1.19+ and the version check can be removed when we drop support for 1.18.

Copy link
Contributor

Choose a reason for hiding this comment

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

Im fine with that. We then have solved the issue for over 75% of the servers atleast

@Sfiguz7 Sfiguz7 added the 🎯 Needs testing This Issue needs to be tested by our team to see if it can be reproduced. label Dec 7, 2023
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.

Looks fine but just to be safe I added a test request. @Slimefun/bug-testers if you could please confirm this works in 1.19+ and creates no other issues we might have missed that would be awesome

@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 Dec 8, 2023
Copy link
Member

@variananora variananora left a comment

Choose a reason for hiding this comment

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

LGTM, build test passed. Tested on Paper 1.19.4-550 and Paper 1.20.2-318

@Sfiguz7 Sfiguz7 merged commit 1a71d83 into Slimefun:master Dec 8, 2023
19 checks passed
@iTwins iTwins deleted the fix/sensitive_blocks branch December 8, 2023 22:36
@Sfiguz7 Sfiguz7 mentioned this pull request Dec 9, 2023
7 tasks
@WalshyDev WalshyDev mentioned this pull request Dec 23, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Signs attached to custom blocks don't drop Block Placer makes blocks vanish
4 participants