Civcraft / Citadel

Do not open issues here; open them on the maintained fork @ DevotedMC
https://github.com/DevotedMC/Citadel
BSD 3-Clause "New" or "Revised" License
6 stars 23 forks source link

Just a guess, but I think our block burn logic is to blame #112

Closed ProgrammerDan closed 9 years ago

ProgrammerDan commented 9 years ago

https://github.com/Civcraft/Citadel/blob/master/src/vg/civcraft/mc/citadel/listener/BlockListener.java#L244

We don't check if the material being burned can be burned before we issue citadel damage. Basically, all reinforced blocks and be broken by lava if I'm reading this right, assuming that Bukkit is issuing a BlockBurn event regardless of Material.isBurnable()

Second opinion/investigation needed. Working theory only.

WildWeazel commented 9 years ago

BBE should only be issued when Bukkit thinks a block has been destroyed by fire. If lava can't normally destroy it, we shouldn't get one.

ProgrammerDan commented 9 years ago

That's my thinking too, but -- have we ever validated this? Either by tracing the spigot code or bukkit code? It's literally the only thing I could think of that would actually impact Citadel reinforcement and is completely unchecked in our event logic.

ttk2 commented 9 years ago

we should probably check it, supposedly its not happening on Civtest so I am further confused.

rourke750 commented 9 years ago

Civtest would have citadel too though right?

ProgrammerDan commented 9 years ago

FWIW: https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/src/main/java/org/bukkit/craftbukkit/event/CraftEventFactory.java#771

Bukkit isn't checking if the block ignited is flammable before passing the event along to the plugin manager.

This too: https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/browse/nms-patches/BlockFire.patch

No checks, even before "fading" -- turning a block to air. I wonder if Spigot is turning blocks to air but ignoring the type or reinforcements. Hoping Anon tests for me.

ttk2 commented 9 years ago

so is spigot actually doing this?

ProgrammerDan commented 9 years ago

I can't tell. I think it's Minecraft. I got some screenshots from one of the reporters showing stone on fire. Legit. Makes me wonder. I've got a ton to investigate on this vein but first I'll be making a pull later tonight to just instrument burn/fade/ignite events in humbug and dump the types to the console. I'll have it on civtest later and build some example structures based on reported issues. See what I can catch.

ttk2 commented 9 years ago

I would be surprised if other servers did not report this to spigot right away though.

ProgrammerDan commented 9 years ago

I've been scouring the spigot bug report tracker, no dice. I found an old issue from long ago about disappearing blocks on chunk load/unload but nothing related to fire burning away non-flammables.

It could be just like in our case, these instances are only now emerging, and we might be on the "leading edge" of folks impacted.

Especially if this is actually a 1.8.7 issue, the number of servers running this at scale is very small, we're kind of bleeding edge at this point.

ttk2 commented 9 years ago

oh fun, if you can find the bad logic though it should be a quick fix once the report is in.

ProgrammerDan commented 9 years ago

Just an update, I'm writing a version of Humbug I'm going to put on Civtest that monitors burn, fade, and ignite events. Then I'm going to set up some control tests and see if I can reproduce and honeypot any issues.

If I get nothing, I'll be pretty convinced it's the chunk corruption issue related to flowing liquids. If so, I'll raise a regression issue with Spigot.

ttk2 commented 9 years ago

sounds good, its a shame this is eating up so much time.

ProgrammerDan commented 9 years ago

After running the humbug variant all day and testing the procedures outlined by the spigot bug, I can't reproduce either a citadel bug or a spigot bug. I'll probably just have to spend some time at the affected areas in game, see what I can see.

ttk2 commented 9 years ago

if you want we could put that debugging version live?

ProgrammerDan commented 9 years ago

There's still a lot I don't know about fire logic in MC/Spigot. I've got some more stuff to try out before I feel ready to put my current hack on civmain :)

At least in terms of this specific issue, I'm inclined to suspect the Spigot regression theory, but I can't reproduce it on Civtest either.

ProgrammerDan commented 9 years ago

Still pretty sure this is a spigot issue. Seems players found workarounds. Closing.