search

SpongePowered / SpongeForge

A Forge mod that implements SpongeAPI
http://www.spongepowered.org/
MIT License
1.14k stars 306 forks source link

Piston duplication blocks issue #2443

Closed noremac09 closed 5 years ago

noremac09 commented 6 years ago

I am currently running

Issue Description https://youtu.be/akmr2KRE8og.

noremac09 commented 6 years ago

I wrote with this problem to the developers of GriefPrevention, they said that the problem is not in their plugin, and said to write here.

Shybella commented 6 years ago

Is this only happening inside of a GriefPrevention claim?

noremac09 commented 6 years ago

No. It seems to me that during block duplication there is some microlag on the server. How can I gather more information for developers? I am well aware that the video is almost nothing clear. Why even look for a mistake?

I also came to the conclusion that this bug does not work on an empty server.

Shybella commented 6 years ago

https://www.youtube.com/watch?v=rH83o5Bo3nU&feature=youtu.be

I'm using SF 3440. I'm unable to reproduce. I have 20 players online too, not empty.

Correction:

http://prntscr.com/kvvvon

It does duplicate. Can confirm.

gabizou commented 6 years ago

Is panda redstone enabled or disabled?

noremac09 commented 6 years ago

Enabled. I just checked this bug with Panda Redstone disabled, still duplicating blocks :(

gabizou commented 6 years ago

I just checked this bug with Panda Redstone disabled, still duplicating blocks :(

Just making sure it's not related (reduces the amount of mixins I have to thread through to find the dupe).

Overall, is there any faster way to reproduce the dupe? I'm assuming there's some math involved somewhere if this is extremely time related.

noremac09 commented 6 years ago

The bug reproduces with both enabled and disabled panda-redstone.

I don't know how to reproduce this dupe faster.

If there are any tools that can help in collecting information? I'll put them on my server and we can collect more information. I have a great online and it's ideal to reproduce when there are people on the server.

noremac09 commented 6 years ago

Update: works with 3458 build.

TheNullablePrototype commented 6 years ago

A piston(not sticky) also duplicates the blocks in the case of the Shulker.

Shybella commented 6 years ago

https://github.com/SpongePowered/SpongeForge/issues/2346

Shulker has tons of dupes, for some reason that's marked as mod incompatibility but happens without any mods but spongeforge.

Rasgnarok commented 6 years ago

Having the same issue with latest recommended and above, https://cdn.discordapp.com/attachments/471732400112795648/494966331125399552/2018-09-27_13.19.17.png. Has there been any debug proposed?

I have panda-redstone enabled, and GP. The piston head seems to be invisible, yet still activates and pushes the shulkerbox, causing duplication into other blocks. Let me know if I can debug in a better way.

gabizou commented 6 years ago

I still have yet to find a way to reproduce this issue.

If the issue can be reproduced with just SpongeForge, I need to know how it can be reproduced with just SpongeForge (no GP interactions, no forge mod interactions), disable panda Redstone, note whether the TPS is struggling, whether there's no lag, etc. As of right now I still can't reproduce the dupe locally to be able to find out why it's duplicating.

I've had thoughts that it's potentially to do with dropped tick updates somehow. Short of making a specialized SpongeForge + map + custom built in debugging for the very specific information of when a block is pushed forward beyond the original block, but writing all of that takes more time than I have at this very moment.

Shybella commented 6 years ago

The tick rate does affect if it duplicates.

gabizou commented 6 years ago

The tick rate does effect if it duplicates.

So... what sort of tick rate do we need to see the duplication occur? As you can imagine, slowing down the tick rate by itself doesn't do much for us, but the possibility of skipping scheduled updates (which pistons are crazy about) could potentially be the root of the issue where pistons are concerned.

Shybella commented 6 years ago

You can create a artificial source with 300 chickens in a 1x1. I have it happen nearly every-time on garbage collections.

Shiunin commented 6 years ago

Hello, I can reproduce this issue. Was directed here after chatting with someone from GP.

Same as OPs replication. Block is pushed by piston out of GP Claim causes block to be duplicated.

I am currently running something a bit older then OP however:

Minecraft: 1.12.2 SpongeAPI: 7.1.0-SNAPSHOT-7105dfc SpongeForge: 1.12.2-2705-7.1.0-BETA-3392 Minecraft Forge: 14.23.4.2705 OS: Debian 9.4

Plugins/Mods: Plugins (26): Minecraft, Minecraft Coder Pack, SpongeAPI, SpongeForge, FastAsyncWorldEdit, AdamantineShield, ArmorStandTools, DailyRewards, EconomyLite, GriefPrevention, Holograms, LuckPerms, MCClans, Nucleus, Nucleus API, PixelBuiltQuests, PixelmonEconomyBridge, PlayerShopsRPG, PokeDisguise, PokexpMultiplier, Punish, SlotMachines, TeslaCore, TeslaCrate, TeslaLibs, WorldEdit

Mods (13): Minecraft, Minecraft Coder Pack, Forge Mod Loader, Minecraft Forge, SpongeAPI, SpongeForge, Gameshark, Pixel Extras, Pixelmon, PokeDisguiseMod, PokeDisplay, Trainer Item, Wondertrade

phit commented 6 years ago

@Shiunin retest with latest first please

Shiunin commented 6 years ago

@Shiunin retest with latest first please

I don't think we're in a place to update right now. I can report back when we do. Just wanted to make you aware it's also impacting our build :)

noremac09 commented 6 years ago

Works, https://imgur.com/a/vHXqmrJ

beta-3480

gabizou commented 6 years ago

@noremac09 how are you reproducing it? Need specifics as far as world setup, plugin setup, etc. Is lag involved etc.

For anyone reading this thread, I full well understand and expect the issue still exists, but without proper documentation on how the dupe happens, I can't troubleshoot or even start targeting any bit of code to try and resolve it.

gabizou commented 5 years ago

For those reading the thread, I’ve already figured out the cause, but a solution to be put into place is rather difficult without changing some things in the api to support more complicated u derstandings of what is actually happening. The root of the issue is that multiple block changes occur at the same block position during a phase state, and therefor, multiple block change types are being exposed as incorrect final types, think the block break of a block, then the block placement of a piston block, then the block change from piston to extended piston, etc.

johnfriedrich commented 5 years ago

Well, due to that bug all SpongeForge servers have to disable pistons entirely. And there are alot people that see pistons as a core-minecraft mechanic. Even if it's a breaking change, no one would care, when it solves this important issue

gabizou commented 5 years ago

It is not so much of an issue with spongeforge that is causing this bug, it’s a combination of spongeforge providing partial information that grief prevention is taking at face value. GP can workaround the bug..

And by breaking change, I’m not talking about a breaking change to the API, I’m talking about not breaking any other core mechanics that could potentially cause the entirety of red stone to react differently, as previously experienced by early versions of the CauseTracker back in 1.10 and before.

johnfriedrich commented 5 years ago

Yes, you can probably workaround almost every missing feature/bug, but blood says sponge has to fix it and he is one of the Sponge Devs. Why should I tell especially him, to add a workaround in GP?

In my opinion, fixing a 100% working and easily discoverable dupe bug to dupe everything in milliseconds is more important than potentially breaking specific redstone mechanics. And I am pretty sure that a fix doesn't break 99,9% of all redstone buildings. As most buildings are simple redstone door mechanisms. Might break very complex stuff, but thats the very minority of all buildings. Sorry, but I don't understand why keeping the bug just to protect maybe a handful very complex redstone buildings

JBYoshi commented 5 years ago

The root of the issue is that multiple block changes occur at the same block position during a phase state

Is there any reason we can't simply have a Map tying block positions to block states? That would ensure only one overall block change gets tracked for each phase state. If A is broken and B is placed in the same location at the same time, I don't think plugins would care about the air block that shows up between the changes.

Rasgnarok commented 5 years ago

Currently the only way to prevent it is disabling pistons entirely. So either way, the current situation is that redstone contraptions, in which pistons play a huge part, get disabled too.

It's a Sponge bug, and asking blood to patch it in GP when it could be addressed in Sponge is not thinking ahead. Since the latest recommended is already slated to prioritize bug-fixes over important API advancement, would it not be better to make redstone in general finally safe to use? Right now all we have as a choice is to blanket ban all pistons, such as item-banning them to turn any owned pistons to dirt, which is likely the worse way to deal with it.

Shybella commented 5 years ago

I remember people tearing me apart when I said this was related and can be fixed with GP.

Good to know though, my fix is just tracking excessive piston movements and alerting staff. Banning the piston isn't because of the bug, it's because its easy. It's easy to also make a fix to help prevent it.

Rasgnarok commented 5 years ago

Yep, its easy to blanket ban and wait for Sponge to address it. And for 90% of the servers, its the only choice they have. Would be far better if it was fixed before we just can't use pistons on Sponge, period.

Rasgnarok commented 5 years ago

Here's a clearer way to reproduce: https://gyazo.com/d7e9a8fe6814f9743fec6e4c289d9a7c.gif

bloodmc commented 5 years ago

Someone provide a test world where it can be reproduced so I can add a workaround until we figure out a proper solution in Sponge.

gabizou commented 5 years ago

Almost completed a potential fix for all of this. @Rasgnarok has mentioned something that may play into this issue: @bloodmc apparently GP will skip events to save performance sometimes? I'll be able to fix the actual events occurring for pistons getting the correct change flags set, but I don't know if that will fix events being skipped by GP due to lag...

For reference, a work in progress capture object to keep a track of multiple positions or staying the same as previous capturing for multiple positions: https://gist.github.com/gabizou/f8963e68a2bdc97cdb8cb70188e75f2e

noremac09 commented 5 years ago

image

image

wat.... with this mod it does not dupe: https://minecraft.curseforge.com/projects/tiquality Who has a problem with dupe, check with this mod.

@Shybella @Shiunin

johnfriedrich commented 5 years ago

This mod is only a bandaid fix that does not fix the underlying problem. Dupes are essential more often if the server lags and this mod tries to prevent lags due to evenly divide resources. That is probably why duping is less likely with this mod

XakepSDK commented 5 years ago

Fix is WIP, just wait or help with it. https://github.com/SpongePowered/SpongeCommon/pull/2163

bloodmc commented 5 years ago

Fixed in GP commit https://github.com/MinecraftPortCentral/GriefPrevention/commit/8d76d1d2c8c04e7a87c880e6df102c220336b606

The conflict with GP is due to Sponge sending block events during piston moves which should be fixed when issue SpongePowered/SpongeCommon#2163 is done. GP already had a bandaid fix for this but there was a bug causing pistons to get through.

Rasgnarok commented 5 years ago

Update on this ticket, testing with SF 3392 (to keep a similar test pattern) and GP 673, seems fixed. No dupe to report after spamming. Shulker boxes seem to also act normally (they drop instead of move). Will confirm this with latest SF before calling it sorted.

Keaton1188 commented 5 years ago

Can confirm that this is still an issue with Sponge version - 1.12.2-2768-7.1.6-RC3561

untitled

I know a pic is useless but this is what a user has reported to me.

Shybella commented 5 years ago

What version of GP are you using? /gpversion

Keaton1188 commented 5 years ago

I don't use GP.

johnfriedrich commented 5 years ago

Are you using any other kind of protection plugin?

Keaton1188 commented 5 years ago

Yep, we are using Nations.

gabizou commented 5 years ago

I'm very close to having this resolved. Cancelling pistons in my branch/pr is working, however, there's a few side effect bugs that I need to fix (mostly notification propagation isn't taking place as it should in some cases). Can refer to https://github.com/SpongePowered/SpongeCommon/pull/2163 for updates and changes.

ImMorpheus commented 5 years ago

PR merged