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 OnAllRemovedFromWorld only triggering once #15680

Merged
merged 1 commit into from Jan 22, 2019

Conversation

Projects
None yet
3 participants
@abcdefg30
Copy link
Member

abcdefg30 commented Oct 4, 2018

We trigger all the other triggers like OnRemovedFromWorld multiple times, except the "on killed" ones (obviously) and the "on captured" triggers (I could fix those as well, we usually don't need this however).

@abcdefg30 abcdefg30 force-pushed the abcdefg30:onAllRemoved branch from 29f3676 to 4799c97 Oct 7, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member Author

abcdefg30 commented Oct 7, 2018

Rebased.

@pchote pchote added this to the Next Release milestone Nov 25, 2018

@pchote
Copy link
Member

pchote left a comment

Code changes LGTM, just one minor nit.

Can you please provide a testcase that demonstrates the bug and fix?

Show resolved Hide resolved OpenRA.Mods.Common/Scripting/Global/TriggerGlobal.cs Outdated
@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 9, 2018

Ping @abcdefg30

@abcdefg30 abcdefg30 force-pushed the abcdefg30:onAllRemoved branch from 4799c97 to 715a65a Dec 18, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 20, 2018

Do you have a testcase that demonstrates the bug and fix?

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 31, 2018

Deferring to Next + 1 so this won't hold up the playtest. We can pull this back if someone can provide a testcase so we can merge this in time.

@pchote pchote modified the milestones: Next Release, Next + 1 Dec 31, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member Author

abcdefg30 commented Jan 7, 2019

Testcase: Replace SetupAlliedUnits in Allies03b with the following

SetupAlliedUnits = function()
	Actor.Create("e7", true, { Owner = player, Location = TanyaWaypoint.Location + CVec.New(0, 2), Facing = 128 })
	Tanya = Actor.Create(TanyaType, true, { Owner = player, Location = TanyaWaypoint.Location, Facing = 128 })
	Rifle1 = Actor.Create("e1", true, { Owner = player, Location = TanyaWaypoint.Location + CVec.New(0, 1) })
	Rifle2 = Actor.Create("e1", true, { Owner = player, Location = TanyaWaypoint.Location + CVec.New(1, 0) })

	if TanyaType == "e7.noautotarget" then
		Trigger.AfterDelay(DateTime.Seconds(2), function()
			Media.DisplayMessage("According to the rules of engagement I need your explicit orders to fire, Commander!", "Tanya")
		end)
	end

	Camera.Position = Tanya.CenterPosition

	Trigger.OnAllRemovedFromWorld({ Tanya, Rifle1, Rifle2 }, function()
		Media.DisplayMessage("Retreat is not an option!")
	end)

	InsertionHeli.Wait(DateTime.Seconds(3))
	InsertionHeli.UnloadPassengers()

	Tanya.Wait(DateTime.Seconds(1))
	Tanya.EnterTransport(InsertionHeli)

	Trigger.AfterDelay(DateTime.Seconds(5), function()
		Rifle1.EnterTransport(InsertionHeli)
		Rifle2.EnterTransport(InsertionHeli)

		Trigger.AfterDelay(DateTime.Seconds(5), function()
			Tanya.EnterTransport(InsertionHeli)
		end)
	end)

	--InsertionHeli.Move(InsertionHeliExit.Location)
	--InsertionHeli.Destroy()

	Trigger.OnKilled(Tanya, function() player.MarkFailedObjective(TanyaSurvive) end)
end

You won't have do anything other than watching for the message "Retreat is not an option!". It should only appear once one Tanya and both rifles are inside the transport. (On bleed you will see the message with only the two rifles inside the transport but both Tanyas outside.)

@abcdefg30 abcdefg30 force-pushed the abcdefg30:onAllRemoved branch from 715a65a to cfdf191 Jan 7, 2019

@pchote pchote modified the milestones: Next + 1, Next Release Jan 7, 2019

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jan 22, 2019

Thanks, LGTM.

@pchote

pchote approved these changes Jan 22, 2019

@pchote pchote merged commit 3e93242 into OpenRA:bleed Jan 22, 2019

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:onAllRemoved branch Jan 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment