SpongePowered / SpongeForge

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

[1.8.9] Explosion transactions include AIR - can be several hundreds for each real block transaction needed) #669

Closed theboomer closed 7 years ago

theboomer commented 8 years ago

Would it be possible to eliminate the inclusion of AIR blocks from the transaction list for an ExplosionEvent.Detonate, in order to reduce an iterable list of impacted blocks down to something meaningful without having to filter-out all the air from a MASSIVE list each time? Or is there a purpose for including them?

The inclusion of the unchanged AIR makes no sense whatsoever that I can see: It does not change states from Air-alpha to Air-beta form. Transactions are not merely a list of all coordinates within the explosion radius, or else we'd have unchanged obsidian->obsidian and stone->stone transactions for blocks where the detonation wave was attenuated.

So can the AIR be filtered out from being stuffed into the transaction list for the explosion - and we just go on assuming that air remains air both within and outside of the blast radius?

 Transaction{original=SpongeBlockSnapshot{worldUniqueId=9a9c00f2-e64b-4443-9f44-a6ed51dce52d, position=(13, 69, 262), blockState=minecraft:air, extendedState=minecraft:air}, default=SpongeBlockSnapshot{worldUniqueId=9a9c00f2-e64b-4443-9f44-a6ed51dce52d, position=(13, 69, 262), blockState=minecraft:air, extendedState=minecraft:air}, custom=null, valid=true}
 Transaction{original=SpongeBlockSnapshot{worldUniqueId=9a9c00f2-e64b-4443-9f44-a6ed51dce52d, position=(12, 69, 262), blockState=minecraft:air, extendedState=minecraft:air}, default=SpongeBlockSnapshot{worldUniqueId=9a9c00f2-e64b-4443-9f44-a6ed51dce52d, position=(12, 69, 262), blockState=minecraft:air, extendedState=minecraft:air}, custom=null, valid=true}
 Transaction{original=SpongeBlockSnapshot{worldUniqueId=9a9c00f2-e64b-4443-9f44-a6ed51dce52d, position=(11, 69, 262), blockState=minecraft:air, extendedState=minecraft:air}, default=SpongeBlockSnapshot{worldUniqueId=9a9c00f2-e64b-4443-9f44-a6ed51dce52d, position=(11, 69, 262), blockState=minecraft:air, extendedState=minecraft:air}, custom=null, valid=true}
 Transaction{original=SpongeBlockSnapshot{worldUniqueId=9a9c00f2-e64b-4443-9f44-a6ed51dce52d, position=(18, 69, 262), blockState=minecraft:air, extendedState=minecraft:air}, default=SpongeBlockSnapshot{worldUniqueId=9a9c00f2-e64b-4443-9f44-a6ed51dce52d, position=(18, 69, 262), blockState=minecraft:air, extendedState=minecraft:air}, custom=null, valid=true}
 Transaction{original=SpongeBlockSnapshot{worldUniqueId=9a9c00f2-e64b-4443-9f44-a6ed51dce52d, position=(17, 69, 262), blockState=minecraft:air, extendedState=minecraft:air}, default=SpongeBlockSnapshot{worldUniqueId=9a9c00f2-e64b-4443-9f44-a6ed51dce52d, position=(17, 69, 262), blockState=minecraft:air, extendedState=minecraft:air}, custom=null, valid=true}

= = = = = = = = = = =

Some numbers that give consideration: Average explosion on flat natural ground (some dirt over stone generally, some grass/flowers present): 440 AIR from 575 total transactions (77% AIR) Average explosion on flat natural ground raised on one block of dirt: 725 AIR from 825 total transactions (88% AIR) Average explosion on flat obsidian floor 575 AIR from 575 total transactions (100% AIR) Average explosion on one flat obsidian raised on one block of dirt: 724 AIR from 725 total transactions (~100% AIR)

Average explosion occuring at height, such as a primed tnt entity flung through the air from a chain reaction, surrounded entirely by air: 1150 AIR from 1150 total transactions -- maybe a handful of solids if its near enough to trees or elevated terrain while still detonating in the air.

ryantheleach commented 8 years ago

I'd prefer the air to stay. I had considered a plugin where instead of damaging blocks creepers would instead spam blocks randomly within the explosion radius. removing the air would make this more problematic as I would need to re-raytrace.

However it's a lot of blocksnapshots that wouldn't otherwise need to be created, so I can see sense in removing the air for performances sake.

Transactions are not merely a list of all coordinates within the explosion radius, or else we'd have unchanged obsidian->obsidian and stone->stone transactions for blocks where the detonation wave was attenuated.

Either the unchanged obsidian and stone should be added, or the air removed. Adding unchanged obsidian would also make it lot easier for "obsidian destruction" plugins.

Zidane commented 8 years ago

At first I was going to close this but elected to think about it for a bit.

After doing so, @theboomer is correct but also incorrect at the same time. The transactions in ChangeBlockEvent should represent an actual change occurring. If an explosion hits AIR then the result is AIR...no change occurs. What is happening in the base game is the Explosion notifies those AIR blocks (but nothing comes of it) and breaks other blocks that aren't AIR. I think the correct solution for this would be to have the list of transactions (ChangeBlockEvent parent) and then have Detonate also give you the blocks it will notify. That way its very simple to see what is going on (as we want to be able to let people know when something notifies so logic can be run on AIR blocks touched by explosions). Might need more thoughts on the matter...

@bloodmc Could use your thoughts here.

theboomer commented 8 years ago

So... the reason for keeping air blocks is to issue notify-neighbor events and trigger any intact BUD configurations that aren't destroyed of the airblock beside it being .... touched by an explosion, yet, not actually updated or modified in any way that a notify-neighbor would trigger? What is the difference for a notify on unchanged air blocks vs unchanged stone blocks?

My key thinking was that an air-surround detonation will have a huge number of transactions -- most creeper explosions will be surface-bound and originate above the ground resulting in lots of air blocks, but thats a discrete event and sure, no problem.. But when multiple TNT are detonated in a pile, the first ones will destroy most of the blocks around, resulting in a lot of air space, as well as propelling some blocks into the air, which is all air surround. When there are lots of TNT in a pile, fired off simultaneously or in a chain reaction, this is some of the busiest work being done on the server already, esp if there are plugins checking each block location for ownership or region info, etc -- and at such a time when there is already a lot of explosions to check, tnt is going to be popping off surrounded by mostly air, resulting in huge bursts of numbers of air->air blocks being stuffed into the transactions. Basically, the more that the server is going to be popping off explosion events back to back to back, the higher the probability that it will only be returning air and no actual block damages, and off shoots the amount of AIR blocks into the transactions - when the server is going to be doing a huge amount of simultaneous (and traditionally, lag-inducing) processing, it ends up scaling up the 'unnecessary Air->Air changes'

Other than the aforementioned "had a plugin idea to make creepers fill the space with random blocks" what kind of systems would need to 'react' to an air block notification, and would this nofify-list then require the solid blocks that are unchanged because they too were "touched, but not altered in any way by the explosion" as the air was touched-but-unaltered?

Having two lists - one of all solids damaged and the other of all volumes that are 'touched by the explosion' esp if that list contains the first - adds even MORE work to the event data generation, even though it would dramatically cut down on the plugin-handling side of filtering through the air noise for 99.99% of explosion-handling processes - if it was a separate list of all unchanged-blocks for whatever reason, such that the entire orbsphere of explosion-zone is recorded in two subsets with no overlap: changed and nonchanged, then that would at least give a small set of block changes and a big set most plugins would be able to ignore, reducing the plugin-processing side of things at least.

Zidane commented 7 years ago

1.8.9 is no longer supported. Check this against stable-5 and re-open if still an issue.