Don't convert back and forth between Explosion objects and their unpacked data. #1368

JBYoshi · 2017-05-24T02:56:00Z

This PR:

Inlines newExplosion() in MixinWorld.triggerExplosion()
Moves the explosion code in MixinWorldServer from triggerExplosion() and newExplosion() to triggerExplosionInternal()
Corrects a few defaults in SpongeExplosionBuilder that badly messed up my testing

gabizou

Some cause tracking details are missing in my opinion.

gabizou · 2017-05-24T04:18:32Z

src/main/java/org/spongepowered/common/mixin/core/world/MixinWorldServer.java

-        // Sponge End
+        // Sponge start - move this behavior to another method to allow precreated explosions to be used
+        // All of the original behavior is in triggerExplosion().
+        triggerExplosion((org.spongepowered.api.world.explosion.Explosion) explosion, Cause.of(NamedCause.source(entityIn == null ? this : entityIn)));


I'd rather use the CauseTracker to define what Cause to use for the explosion source. Since the source passed in as Entity is usually an entity ticking, or in some cases a TileEntity passing null for explosions. You may also want to verify the explosion optimization (not really optimization, it's an optimization for clients on the server due to the unperformant explosion packets and what they entail) still functions as expected.

Fixed. Although pretty much the entire PR changed - you may need to re-review it.

Zidane · 2017-08-19T23:23:43Z

src/main/java/org/spongepowered/common/mixin/optimization/MixinWorldServer_Explosion.java

@@ -48,21 +50,23 @@
 @Mixin(value = WorldServer.class, priority = 1111)
 public abstract class MixinWorldServer_Explosion extends MixinWorld {


@gabizou @JBYoshi Any reason why this mixin is on its own and not in MixinWorldServer?

It looks like it's separate because it's a configurable optimization.

@Zidane because it's altering the mechanics of multiple explosions occurring at the same time. This goes about removing the point of sending explosion packets (reducing lag on clients), this was meddled with @amaranth a while back but I haven't verified whether it should be enabled by default now or not.

JBYoshi · 2017-10-30T01:32:20Z

Can this be merged?

liach · 2017-10-30T04:32:56Z

src/main/java/org/spongepowered/common/event/tracking/phase/plugin/PluginPhase.java

 import org.spongepowered.common.event.tracking.phase.TrackingPhase;
+import org.spongepowered.common.event.tracking.phase.general.ExplosionContext;


Extra import?

Whoops :P I will revert that.

Actually, this was intended. It now uses this class instead of the previous ExplosionContext class that was in this package.

liach · 2017-10-30T04:33:20Z

src/main/java/org/spongepowered/common/event/tracking/phase/plugin/PluginPhaseState.java

@@ -29,7 +29,7 @@
 import org.spongepowered.common.event.tracking.phase.TrackingPhase;
 import org.spongepowered.common.event.tracking.phase.TrackingPhases;

-public abstract class PluginPhaseState<P extends PluginPhaseContext<P>> implements IPhaseState<P> {
+public abstract class PluginPhaseState<P extends PhaseContext<P>> implements IPhaseState<P> {


This was to allow CustomExplosionState to use the same context class as ExplosionState (of GeneralPhase); there's pretty much no difference between the two types.

JBYoshi · 2017-12-02T23:17:08Z

@liach I found a way to make this work without messing with the PhaseContext class hierarchy. The callers now create a context and pass the explosion to it.

ryantheleach · 2018-01-30T06:06:53Z

~qa

@gabizou You offered review advice earlier, so I'm looking to see a final review to see if it's ready, or else give to someone else.

@JBYoshi I'll make a test plugin that tests the original issue tonight.

phit · 2018-04-19T23:28:18Z

@ryantheleach status?

ryantheleach · 2018-04-20T04:35:18Z

I know that I said previously that I was going to make a test plugin, but at this stage I have no idea if I did that or not, I've been somewhat MIA over the last few months.

At present anything that's blocked on me should be considered up for fair game until I can find the time to invest back into Sponge.

ImMorpheus · 2018-10-13T15:44:19Z

Updated (after asking @JBYoshi about it).

Note: PluginPhase.State.CUSTOM_EXPLOSION was removed by 0e16a5d#diff-789d0465f5f0c54bb475b2958e6263d6 so I'm using GeneralPhase.State.EXPLOSION.

gabizou

Looks good to me. Just may need to add explosion event ShouldFire checks to reduce potential stack frame creation unnecessarily.

ImMorpheus · 2019-02-01T15:52:51Z

@gabizou I'll add it with other performance improvements once I reopen the explosions part of #2031

ImMorpheus · 2019-02-01T17:10:15Z

Conflicts fixed and rebased.

A note (In case some plugins complain about this being a breaking change): the default value of shouldBreakBlocks (and shouldDamageEntities) is an implementation detail, it's not mentioned in the API. If you're creating an explosion with the ExplosionBuilder without explicitly setting those flags, then it's your fault for relying on the default value.

…cked data. Fixes SpongePowered#1367

…cked data. (SpongePowered#1368) Fixes SpongePowered#1367

gabizou requested changes May 24, 2017

View reviewed changes

gabizou added system: phase tracker system: event version: 1.11 (u) API: 6 (unsupported since Jan 1st 2018) labels May 24, 2017

JBYoshi force-pushed the fix/explosion-reuse-existing branch 4 times, most recently from 149d94f to 100a236 Compare May 25, 2017 02:43

JBYoshi added status: needs review a code review is needed status: needs updating hey, origin changed, you need to update and removed version: 1.11 (u) API: 6 (unsupported since Jan 1st 2018) labels Aug 6, 2017

JBYoshi force-pushed the fix/explosion-reuse-existing branch from 100a236 to f57951d Compare August 6, 2017 03:00

JBYoshi removed the status: needs updating hey, origin changed, you need to update label Aug 6, 2017

Zidane reviewed Aug 19, 2017

View reviewed changes

JBYoshi force-pushed the fix/explosion-reuse-existing branch from f57951d to 0717cce Compare August 27, 2017 03:22

JBYoshi closed this Sep 24, 2017

JBYoshi deleted the fix/explosion-reuse-existing branch September 24, 2017 19:52

JBYoshi restored the fix/explosion-reuse-existing branch September 24, 2017 20:00

JBYoshi reopened this Sep 24, 2017

JBYoshi force-pushed the fix/explosion-reuse-existing branch from 0717cce to 1fe0d7d Compare October 3, 2017 21:37

JBYoshi requested a review from Deamon5550 as a code owner October 3, 2017 21:37

Deamon5550 removed their request for review October 4, 2017 00:16

JBYoshi force-pushed the fix/explosion-reuse-existing branch from 1fe0d7d to 46397a7 Compare October 12, 2017 01:10

liach reviewed Oct 30, 2017

View reviewed changes

JBYoshi force-pushed the fix/explosion-reuse-existing branch 2 times, most recently from ce3eb47 to d563de7 Compare December 2, 2017 23:13

ryantheleach added this to Acknowledged in gabizou's TODO List via automation Jan 30, 2018

limbo-app added the status: needs testing does this run, does it solve the issue etc label Jan 30, 2018

JBYoshi force-pushed the fix/explosion-reuse-existing branch from d563de7 to a36842d Compare February 4, 2018 00:36

JBYoshi requested review from Faithcaio and kashike as code owners February 4, 2018 00:36

JBYoshi changed the base branch from bleeding to stable-7 February 4, 2018 00:36

ImMorpheus added type: bug Something isn't working 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 29, 2018

ImMorpheus force-pushed the fix/explosion-reuse-existing branch from a36842d to 688b8a5 Compare October 13, 2018 15:39

ImMorpheus added status: needs review a code review is needed status: needs testing does this run, does it solve the issue etc and removed status: needs updating hey, origin changed, you need to update labels Oct 13, 2018

gabizou approved these changes Feb 1, 2019

View reviewed changes

ImMorpheus force-pushed the fix/explosion-reuse-existing branch from 688b8a5 to ab7b6d8 Compare February 1, 2019 16:43

Don't convert back and forth between Explosion objects and their unpa…

ab7b6d8

…cked data. Fixes SpongePowered#1367

ImMorpheus merged commit a8cddaf into SpongePowered:stable-7 Feb 1, 2019

gabizou's TODO List automation moved this from Acknowledged to Done Feb 1, 2019

ImMorpheus pushed a commit to ImMorpheus/SpongeCommon that referenced this pull request Feb 8, 2019

Don't convert back and forth between Explosion objects and their unpa…

91ea031

…cked data. (SpongePowered#1368) Fixes SpongePowered#1367

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't convert back and forth between Explosion objects and their unpacked data. #1368

Don't convert back and forth between Explosion objects and their unpacked data. #1368

JBYoshi commented May 24, 2017 •

edited

gabizou left a comment

gabizou May 24, 2017

JBYoshi May 25, 2017 •

edited

Zidane Aug 19, 2017

JBYoshi Aug 24, 2017

gabizou Aug 25, 2017

JBYoshi commented Oct 30, 2017

liach Oct 30, 2017

JBYoshi Oct 30, 2017

JBYoshi Oct 30, 2017

liach Oct 30, 2017

JBYoshi Oct 30, 2017

JBYoshi commented Dec 2, 2017

ryantheleach commented Jan 30, 2018 •

edited

phit commented Apr 19, 2018

ryantheleach commented Apr 20, 2018

ImMorpheus commented Oct 13, 2018

gabizou left a comment

ImMorpheus commented Feb 1, 2019

ImMorpheus commented Feb 1, 2019

		@@ -48,21 +50,23 @@
		@Mixin(value = WorldServer.class, priority = 1111)
		public abstract class MixinWorldServer_Explosion extends MixinWorld {

		import org.spongepowered.common.event.tracking.phase.TrackingPhase;
		import org.spongepowered.common.event.tracking.phase.general.ExplosionContext;

Don't convert back and forth between Explosion objects and their unpacked data. #1368

Don't convert back and forth between Explosion objects and their unpacked data. #1368

Conversation

JBYoshi commented May 24, 2017 • edited

gabizou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JBYoshi May 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JBYoshi commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JBYoshi commented Dec 2, 2017

ryantheleach commented Jan 30, 2018 • edited

phit commented Apr 19, 2018

ryantheleach commented Apr 20, 2018

ImMorpheus commented Oct 13, 2018

gabizou left a comment

Choose a reason for hiding this comment

ImMorpheus commented Feb 1, 2019

ImMorpheus commented Feb 1, 2019

JBYoshi commented May 24, 2017 •

edited

JBYoshi May 25, 2017 •

edited

ryantheleach commented Jan 30, 2018 •

edited