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 destroyed truck escaping ra mission sarin-gas-1 #16841

Merged
merged 1 commit into from Oct 4, 2019

Conversation

@fruestueck
Copy link
Contributor

commented Jul 26, 2019

Fixes #16839

Simply adding not actor.IsDead to the condition would prevent first half of 16839.

By checking for actor.IsIdle (and resceduling the Trigger) the truck has to finish its move and trigger a possible mine before that.

Copy link
Member

left a comment

I guess you were setting new triggers because the actor is not idle the first time it enters the destination location. Can we try to add an idle trigger to the truck instead?

mods/ra/maps/sarin-gas-1-crackdown/crackdown.lua Outdated Show resolved Hide resolved
mods/ra/maps/sarin-gas-1-crackdown/crackdown.lua Outdated Show resolved Hide resolved
mods/ra/maps/sarin-gas-1-crackdown/crackdown.lua Outdated Show resolved Hide resolved
@abcdefg30

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Tbh, I think the cleanest solution would be to actually let the trucks move outside of the playable area before removing them. That would be left to another PR though.

@fruestueck

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

Tbh, I think the cleanest solution would be to actually let the trucks move outside of the playable area before removing them.

I tried it but failed with my limited knowledge about the available lua cmds. A simple move to one cell outside the map won't work as far as I got to test it.
What lua function would be suitable?

@abcdefg30

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

What lua function would be suitable?

None as far as I know, hence I said we should leave it to another PR to keep things simple here (imho).

@fruestueck fruestueck closed this Sep 15, 2019
@fruestueck fruestueck reopened this Sep 15, 2019
@fruestueck fruestueck force-pushed the fruestueck:fix-sarin-gas-1-crackdown branch from 616a397 to 72aa118 Sep 15, 2019
@fruestueck

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2019

Fixed and rebased.

Update: Removed duplicate newline

@fruestueck fruestueck requested a review from abcdefg30 Sep 15, 2019
@fruestueck fruestueck force-pushed the fruestueck:fix-sarin-gas-1-crackdown branch from bf23c6f to 7373f1e Sep 16, 2019
Copy link
Member

left a comment

This is not exactly what I had in mind. I thought about replacing the footprint trigger with an idle trigger. Since we know which route each truck is going to take when applying the idle trigger (if you do it after it got the move command), you can check if the truck reached its destination inside the trigger. If it did, the mission is over, otherwise the truck should do another move to the destination.

mods/ra/maps/sarin-gas-1-crackdown/crackdown.lua Outdated Show resolved Hide resolved
@fruestueck

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

Updated.

  • I've combined the MoveTruckNorth and MoveTruckSouth logic in a seperate function as they are pretty the same.
  • I could also merge MoveTruckNorth, MoveTruckSouth, MoveTruckEscapeRoute & SetTruckEscapeTrigger
  • The following line was removed as I'm not sure if it is needed anymore. If someone could explain I'd be happy. Trigger.RemoveProximityTrigger(triggerlose)
  • We could also scratch the check for the escapelocation as the trucks will never go idle until they reach the escape waypoint afaik. If a waypoint is blocked, they just wait until it's unblocked.
@fruestueck fruestueck force-pushed the fruestueck:fix-sarin-gas-1-crackdown branch from 6a15ade to 14daa6e Sep 16, 2019
Copy link
Member

left a comment

Untested, but looks good now!

The following line was removed as I'm not sure if it is needed anymore. If someone could explain I'd be happy. Trigger.RemoveProximityTrigger(triggerlose)

You are right, we don't need that any more. Since you are not setting any triggers, we also don't need to remove them.

I think merging the functions (your second point) would be cleaner, but I don't think it is required, so up to you.

We could also scratch the check for the escapelocation as the trucks will never go idle until they reach the escape waypoint afaik. If a waypoint is blocked, they just wait until it's unblocked.

Usually I would agree. However I think it is good to have some sort of save guard against weird things like #5968 (comment) happening.

mods/ra/maps/sarin-gas-1-crackdown/crackdown.lua Outdated Show resolved Hide resolved
mods/ra/maps/sarin-gas-1-crackdown/crackdown.lua Outdated Show resolved Hide resolved
mods/ra/maps/sarin-gas-1-crackdown/crackdown.lua Outdated Show resolved Hide resolved
@fruestueck fruestueck force-pushed the fruestueck:fix-sarin-gas-1-crackdown branch from 14daa6e to 83465a4 Sep 16, 2019
@fruestueck

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

TruckEscapeRoutes merged and updated.

Copy link
Member

left a comment

Nice cleanup!

@reaperrr

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2019

Since we need (at least) one more playtest anyway, might as well pick this to prep if we get another review in time.

@reaperrr reaperrr added this to the Next Release milestone Sep 28, 2019
@pchote
pchote approved these changes Oct 4, 2019
@pchote pchote merged commit 81eb939 into OpenRA:bleed Oct 4, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.