Skip to content

fix(Rune): Rune effect loss when placed soulbound/fireproof blocks are broken#1063

Open
TagDL wants to merge 7 commits into
pylonmc:masterfrom
TagDL:fix/block-place-break-rune-loss
Open

fix(Rune): Rune effect loss when placed soulbound/fireproof blocks are broken#1063
TagDL wants to merge 7 commits into
pylonmc:masterfrom
TagDL:fix/block-place-break-rune-loss

Conversation

@TagDL

@TagDL TagDL commented Jun 12, 2026

Copy link
Copy Markdown

Fixed a bug where RebarBlocks with Soulbound Rune and Fireproof Rune effects would lose both modifications upon being placed and subsequently broken by players.

@JustAHuman-xD JustAHuman-xD left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We actually have a system for attaching data directly to blocks, instead of using the chunk pdc like you do here!

If you take a look at some of rebar's built-in interfaces you should be able to find some examples so that you can swap to it. (VirtualInventoryRebarBlock, RecipeProcessorRebarBlock, EntityHolderRebarBlock, to name a few)

Additionally the logic for retaining both runes, soulbound & fireproof should be inside of the corresponding classes in a listener, not inside this abstract class here.

When you move it to both I'd also suggest adding a pdc key to the fireproof rune, and checking for that instead of just the datatag, you could also add some util methods for both which you could use for this. (SoulboundRune#isSoulBound, FireproofRune#isFireproof, for ex)

Overall thank you for the PR! I'm sure with a few changes this can definitely be added

@TagDL TagDL requested a review from JustAHuman-xD June 12, 2026 14:35
Comment thread src/main/java/io/github/pylonmc/pylon/content/tools/FireproofRune.java Outdated
Comment thread src/main/java/io/github/pylonmc/pylon/content/tools/FireproofRune.java Outdated
Comment thread src/main/java/io/github/pylonmc/pylon/content/tools/FireproofRune.java Outdated
Comment thread src/main/java/io/github/pylonmc/pylon/content/tools/SoulboundRune.java Outdated
@TagDL TagDL requested a review from JustAHuman-xD June 13, 2026 12:57
@LordIdra

LordIdra commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Tested that this solution fixes rebar blocks, but it does not address vanilla blocks and we definitely should as it's more or less the same issue.

Unfortunately I think we would need to switch back to using chunk's PDC to fix this for normal blocks, not using the approach Justin suggested.

@JustAHuman-xD do you have any input since you reviewed it originally?

@LordIdra

LordIdra commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Tested that this solution fixes rebar blocks, but it does not address vanilla blocks and we definitely should as it's more or less the same issue.

Unfortunately I think we would need to switch back to using chunk's PDC to fix this for normal blocks, not using the approach Justin suggested.

@JustAHuman-xD do you have any input since you reviewed it originally?

Discussed in VC - I think using the chunk's PDC is still the best way forward. Could you change it back to that and handle the vanilla case? Sorry for the confusion

@TagDL

TagDL commented Jul 4, 2026

Copy link
Copy Markdown
Author

Tested that this solution fixes rebar blocks, but it does not address vanilla blocks and we definitely should as it's more or less the same issue.

Unfortunately I think we would need to switch back to using chunk's PDC to fix this for normal blocks, not using the approach Justin suggested.

@JustAHuman-xD do you have any input since you reviewed it originally?

Discussed in VC - I think using the chunk's PDC is still the best way forward. Could you change it back to that and handle the vanilla case? Sorry for the confusion

Already change it to chunk's PDC❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants