Fix a crash when RallyPoint creates RallyPointIndicator

[dnqbob]

Fix #20938

How it happens

When actor with RallyPoint is created by Lua Script, it has a low chance that the RallyPointIndicator is created when effect is ticking (I can only guess so). It was reported by a tester on my mission map testing.

Use this fix and the replay is no longer crash

[dnqbob]

I have the replay and the game project (non-git) used by my tester, but it is very strange that my project in git cannot read the replay (cannot find the map).

The effective game project is now holded by Hans (tester), and we all have that replay file:
Replay.zip

Update:
You can try this link to download the project files.

[abcdefg30]

Why would INotifyCreated.Created run inside the Tick of an effect? That would mean that an effect created an actor.

[dnqbob]

So I can only guess so.

I have the same thought as you said, but the fix is working while I am still confused

[dnqbob]

So this how I debug:

1. use vars in World to storage the latest operation(add/remove) to effects. I confirmed that it was "adding the RallyPointIndicator" cause the crash
2. then, use a var to storage the latest actor with RallyPoint that just add to world. I found that it was the problem of an actor that was created by Lua.

[abcdefg30]

What was triggering the Lua callback that created the actor?

[dnqbob]

Triggered by

	Trigger.OnAnyKilled(Shopkeepers, function(a)
		EnrageMercenaries() ...

[PunkPun]

You shouldn't be adding any actors to world on the lua trigger frame

[dnqbob]

That is weird then. I don't see any description or comment in Lua Trigger about this limitation.

[dnqbob]

meanwhile, if we cannot use call back inside trigger frames to spawn actor, then coding basic story plot like reinforcement after certain mission completed or actor killed will be impossible or too complicated.

[PunkPun]

That is weird then. I don't see any description or comment in Lua Trigger about this limitation.

In the lua scripting openra provides we always make sure that actors are only added to world on end frame tick. Though I suppose rally point creates the actor even before it's added to world?

[dnqbob]

In the lua scripting openra provides we always make sure that actors are only added to world on end frame tick.

No. In Actor.Create(), you create an actor in Lua ticking, and only add it to world at the frame end.

Reinforcement does everything in Lua ticking. Update: Nope, the same as Actor.Create()

[dnqbob]

And at line 206, in Actor.cs, the INotifyCreated is called no matter you add it to world or not.

[abcdefg30]

Yes, that makes sense now. The effect should be added in INotifyAddedToWorld.AddedToWorld instead of Created, see for example

OpenRA/OpenRA.Mods.Cnc/Traits/GpsDot.cs

Lines 43 to 56 in 56b5ace

	void INotifyCreated.Created(Actor self)
	{
	effect = new GpsDotEffect(self, info);
	}
	void INotifyAddedToWorld.AddedToWorld(Actor self)
	{
	self.World.AddFrameEndTask(w => w.Add(effect));
	}
	void INotifyRemovedFromWorld.RemovedFromWorld(Actor self)
	{
	self.World.AddFrameEndTask(w => w.Remove(effect));
	}

[dnqbob]

In addition, the example shows us most of the IEffect should be added to frame end task, instead of adding them in actor tick.

[dnqbob]

I am still very curious about it: this bug is not a 100% crash, or I have to say this is a rare crash, consider OpenRA has so many mods and we don't see crash like this. And I cannot reproduce this crash myself, have tried many times. Only crash in this replay.

Can someone explain in detail on how the timing cause the problem?

Meanwhile, this will lead to a problem that we have to check all of the traits and related places on the timing of effect added&removed.

[abcdefg30]

Automatic checks fail:

RallyPoint must explicitly implement the interface member INotifyAddedToWorld.AddedToWorld
RallyPoint must explicitly implement the interface member INotifyRemovedFromWorld.RemovedFromWorld
Explicit interface violations: 2
make: *** [Makefile:117: check] Error 1

[dnqbob]

Fixed

[Mailaender]

Changelog