Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix actors not properly entering the world after unloading #17133

Merged
merged 3 commits into from Oct 6, 2019

Conversation

@abcdefg30
Copy link
Member

commented Sep 20, 2019

Resolves the specific case of #17009.
This actually looks to be a regression from #16938.

Filing as a draft since I didn't debug/test this much, but want to get some feedback first if this is aiming in the right direction. Ping @tovl.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:unload branch from 45bb707 to 8205307 Sep 22, 2019
@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2019

Updated. This now queues a new MoveIntoWorld activity after cancelling the actor's activities in UnloadCargo.

@tovl removing the CallFunc from the harvester ctor does unfortunately regress harvesters build from war factories that are not produced nearby ore patches (outside the scan radius). On bleed they would pick a refinery and scan from there.

In theory this doesn't fully fix all regressions, since we still cancel the activities. But most of the times that doesn't matter or is not noticeable (at least from what I saw in a few quick tests) and the current version of the PR doesn't leave actors in bogus state. I suppose to "properly" fix this we should insert the MoveIntoWorld activity at the start of the queue, not at the end.

@abcdefg30 abcdefg30 marked this pull request as ready for review Sep 22, 2019
@pchote pchote added this to the Next Release milestone Sep 22, 2019
@pchote

This comment has been minimized.

Copy link
Member

commented Sep 22, 2019

Queuing the MoveIntoWorld as a child of RideTransport would let it run before any other queued activities.

@tovl

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2019

removing the CallFunc from the harvester ctor does unfortunately regress harvesters build from war factories that are not produced nearby ore patches (outside the scan radius).

If the CallFunc is still needed, we can simply queue it in the AddFrameEndTask block before FindAndDeliverResources. This would make sure it is queued after MoveIntoWorld. The same goes for any other hypothetical activity.

(Of course it would be more proper to move this inside FindAndDeliverResources but that is not important here.)

Queuing the MoveIntoWorld as a child of RideTransport would let it run before any other queued activities.

We already do this, and it does. We are talking about units which spawn inside a transport.

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2019

Queueing in an FrameEndTask would "solve" the issue here, but I still think it is bad to just run with the assumption that MoveIntoWorld is the first activity, especially since it is not really obvious to any outside code. (Hence we didn't immediately figure this out after I opened #17009.)

@pchote

This comment has been minimized.

Copy link
Member

commented Sep 22, 2019

I still think it is bad to just run with the assumption that MoveIntoWorld is the first activity

I agree completely with this, and think it would be justified to use a horrible hack (use reflection in Passenger.Created to rewrite the activity queue) to provide a simple solution to enforce this.

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2019

We can do that as (hopefully temporary) solution, although I think simply queuing another MoveIntoWorld is the easier solution.

@pchote

This comment has been minimized.

Copy link
Member

commented Sep 22, 2019

the real (IMO) best solution is to not have traits queue activities for basic and necessary state initialisation. These should be done either from Tick or FrameEndTasks instead to avoid conflicts between other unknown traits (including Passenger, here). Do we have anything aside from Harvester and Passenger that does this atm?

@tovl

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2019

Passenger doesn't currently queue any activities upon creation and I have no idea why it should. As far as I can tell it is only Harvester that is conflicting here.

@pchote

This comment has been minimized.

Copy link
Member

commented Sep 22, 2019

The conflict in Passenger's case is that we cancel the activity queue as part of the transportation process.

The fundamental problem here is assuming that an activity queued with QueueActivity is guaranteed to run at a specific time, even though everything else in the activity system avoids or explicitly breaks that assumption. I gave this a pass in #16938, even though I specifically predicted problems, because I didn't want to butt heads over design. This was a bad idea, and I'm going to insist that we fix this in a more robust way now.

Removing the Harvester's activity queuing doesn't solve the problem, it just punts it to the next time somebody queues an activity from Created without remembering to specifically work around MoveIntoWorld.

We need to invent a way to guarantee that MoveIntoWorld will run first, no matter what any other trait may do. The simplest hack is to use reflection to force MoveIntoWorld to the front of the queue. A cleaner approach would be to define a new interface that returns an activity to be inserted first when created, and throws an exception if multiple traits on the actor try to use it at the same time.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:unload branch 5 times, most recently from 0eda24a to 5d9c461 Sep 22, 2019
@abcdefg30 abcdefg30 force-pushed the abcdefg30:unload branch from 5d9c461 to f871701 Sep 22, 2019
@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Sep 22, 2019

Updated as discussed on IRC.

@abcdefg30 abcdefg30 changed the title Investigate actors not properly entering the world after unloading Fix actors not properly entering the world after unloading Sep 22, 2019
OpenRA.Game/Actor.cs Outdated Show resolved Hide resolved
@pchote

This comment has been minimized.

Copy link
Member

commented Sep 22, 2019

LGTM in theory, but it may be a while before I can test this.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:unload branch from f871701 to 6639739 Sep 23, 2019
@OpenRA OpenRA deleted a comment from woody3407 Sep 24, 2019
OpenRA.Game/Actor.cs Outdated Show resolved Hide resolved
Copy link
Member

left a comment

LGTM otherwise 👍

Copy link
Contributor

left a comment

RA shellmap crashes after a few minutes (same after rebasing on latest bleed):

OpenRA engine version {DEV_VERSION}
Red Alert mod version {DEV_VERSION}
on map 07df5cb37d0193d1069089c6baf8d44baeda70b8 (Desert Shellmap by Scott_NZ).
Date: 2019-10-05 14:19:31Z
Operating System: Linux (Unix 4.15.0.65)
Runtime Version: Mono 6.4.0.198 (tarball Tue Sep 24 01:21:28 UTC 2019) CLR 4.0.30319.42000
Exception of type `System.InvalidOperationException`: An activity was queued before the actor was created. Queue it inside the INotifyCreated.Created callback instead.
  at OpenRA.Actor.QueueActivity (OpenRA.Activities.Activity nextActivity) [0x00008] in <96dc0e61ff104a97bc9b600436e9b4a6>:0 
  at OpenRA.Actor.QueueActivity (System.Boolean queued, OpenRA.Activities.Activity nextActivity) [0x00009] in <96dc0e61ff104a97bc9b600436e9b4a6>:0 
  at OpenRA.Mods.Common.Traits.FallsToEarth..ctor (OpenRA.ActorInitializer init, OpenRA.Mods.Common.Traits.FallsToEarthInfo info) [0x00019] in <e21c599603154466805b3869cd3c3ca3>:0 
  at OpenRA.Mods.Common.Traits.FallsToEarthInfo.Create (OpenRA.ActorInitializer init) [0x00000] in <e21c599603154466805b3869cd3c3ca3>:0 
  at OpenRA.Actor..ctor (OpenRA.World world, System.String name, OpenRA.Primitives.TypeDictionary initDict) [0x000a2] in <96dc0e61ff104a97bc9b600436e9b4a6>:0 
  at OpenRA.World.CreateActor (System.Boolean addToWorld, System.String name, OpenRA.Primitives.TypeDictionary initDict) [0x00000] in <96dc0e61ff104a97bc9b600436e9b4a6>:0 
  at OpenRA.World.CreateActor (System.String name, OpenRA.Primitives.TypeDictionary initDict) [0x00000] in <96dc0e61ff104a97bc9b600436e9b4a6>:0 
  at OpenRA.Mods.Common.Traits.SpawnActorOnDeath+<>c__DisplayClass5_0.<OpenRA.Traits.INotifyRemovedFromWorld.RemovedFromWorld>b__4 (OpenRA.World w) [0x00000] in <e21c599603154466805b3869cd3c3ca3>:0 
  at OpenRA.World.Tick () [0x001a9] in <96dc0e61ff104a97bc9b600436e9b4a6>:0 
  at OpenRA.Game.InnerLogicTick (OpenRA.Network.OrderManager orderManager) [0x001bc] in <96dc0e61ff104a97bc9b600436e9b4a6>:0 
  at OpenRA.Game.LogicTick () [0x0003e] in <96dc0e61ff104a97bc9b600436e9b4a6>:0 
  at OpenRA.Game.Loop () [0x000e3] in <96dc0e61ff104a97bc9b600436e9b4a6>:0 
  at OpenRA.Game.Run () [0x0003c] in <96dc0e61ff104a97bc9b600436e9b4a6>:0 
  at OpenRA.Game.InitializeAndRun (System.String[] args) [0x00010] in <96dc0e61ff104a97bc9b600436e9b4a6>:0 
  at OpenRA.Program.Main (System.String[] args) [0x00044] in <96dc0e61ff104a97bc9b600436e9b4a6>:0 

@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2019

Pushed a fix.

Copy link
Contributor

left a comment

Can't see any regressions wrt unloading ingame.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:unload branch from 80913f1 to d67fb71 Oct 6, 2019
@abcdefg30

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2019

Rebased and updated.

@pchote
pchote approved these changes Oct 6, 2019
@pchote pchote merged commit acea193 into OpenRA:bleed Oct 6, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30 abcdefg30 deleted the abcdefg30:unload branch Oct 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.