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

Include both custom and vanilla DataManipulators in BlockSnapshots. #1905

Closed
wants to merge 1 commit into from

Conversation

JBYoshi
Copy link
Member

@JBYoshi JBYoshi commented May 8, 2018

"Include ALL the manipulators!" - Quote from someone or something, or maybe no one

Fixes https://forums.spongepowered.org/t/getting-sign-lines-from-changeblockevent-break/23461.
Fixes #2416.

I'm worried about side effects, though (for example, if the custom data gets saved off and conflicts with NBT data), so this may need quite a bit of testing.

@JBYoshi JBYoshi added system: data status: needs review a code review is needed status: needs testing does this run, does it solve the issue etc version: 1.12 (u) API: 7 (unsupported since May 21st 2021) labels May 8, 2018
@JBYoshi JBYoshi requested a review from gabizou May 8, 2018 20:16
@gabizou
Copy link
Member

gabizou commented May 8, 2018

The original reason why we used only custom manipulators at the time for BlockSnapshots was over a discussion for performance when retrieving the manipulators for block states and tile entities and the spam that could be affected.

Can introduce some more performant code for block states such that on block state creation they already have all their manipulators and key values assigned (doesn't really increase load time since they'll be done before the game is even loaded).

@ImMorpheus ImMorpheus added status: needs updating hey, origin changed, you need to update and removed status: needs review a code review is needed status: needs testing does this run, does it solve the issue etc labels Aug 30, 2018
@JBYoshi JBYoshi force-pushed the fix/blocksnapshot-tileentities branch from 02959fb to 0a3894f Compare November 17, 2019 23:46
@JBYoshi JBYoshi added status: needs review a code review is needed and removed status: needs updating hey, origin changed, you need to update labels Nov 17, 2019
@dualspiral dualspiral added this to PRs: Held in 7.x LTS Issue Triage Jun 9, 2020
@gabizou
Copy link
Member

gabizou commented Jun 10, 2020

This won't actually fix any of the issues originally requested about getting Data from a TileEntity as if a BlockSnapshot still had a TileEntity in place. Due to the methods being used to efficiently clone as much data into a BlockSnapshot for phase tracking, none of these methods will populate the DataManipulators as you'd expect. The resolution is still to implement NbtProcessors.

@gabizou gabizou closed this Jun 10, 2020
7.x LTS Issue Triage automation moved this from PRs: Held to Complete Jun 10, 2020
@gabizou gabizou deleted the fix/blocksnapshot-tileentities branch June 10, 2020 04:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs review a code review is needed system: data version: 1.12 (u) API: 7 (unsupported since May 21st 2021)
Projects
Development

Successfully merging this pull request may close these issues.

Getting sign lines from block break event returns null
3 participants