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

Find alternative landing spots for blocked reinforcements #13785

Merged
merged 2 commits into from Oct 8, 2017

Conversation

Projects
None yet
5 participants
@abcdefg30
Member

abcdefg30 commented Aug 7, 2017

Fixes units blocking transports from landing. Easy noticeable on carryalls in D2k (e.g. let the first reinforcements in Atreides04 stay and wait for the second reinforcements).

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Aug 7, 2017

Member

Not tested but i think what this does to move the unit below the carryall. I'm not sure if that's really what we would want to do, it is not the behaviour in vanilla game and only works if blocking actor is mobile, not for structures for example. Carryall should change its drop location if it is blocked.

Alternatively, i think you can get a lua value to select which behaviour to use. Not sure which should be default tho.

Member

MustaphaTR commented Aug 7, 2017

Not tested but i think what this does to move the unit below the carryall. I'm not sure if that's really what we would want to do, it is not the behaviour in vanilla game and only works if blocking actor is mobile, not for structures for example. Carryall should change its drop location if it is blocked.

Alternatively, i think you can get a lua value to select which behaviour to use. Not sure which should be default tho.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Aug 7, 2017

Member

Yes, this does move units below the carryall. I think non-mobile blockers are a separate issue (for another PR).

Member

abcdefg30 commented Aug 7, 2017

Yes, this does move units below the carryall. I think non-mobile blockers are a separate issue (for another PR).

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 7, 2017

Member

Expanding on @MustaphaTR's suggestion, a nearEnough parameter combined with a domain check should be a simple and relatively robust way to have the carryall pick a nearby unblocked cell to land in.

Member

pchote commented Aug 7, 2017

Expanding on @MustaphaTR's suggestion, a nearEnough parameter combined with a domain check should be a simple and relatively robust way to have the carryall pick a nearby unblocked cell to land in.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Aug 7, 2017

Member

You could block carryall with units in original tho, making carryall take back off(or not land at all if it was already blocked) and go to another cell. I would prefer that behaviour to apply OpenRA too. You could technically do that infinitely and make carryall fail to place its units.

Pchote's idea isn't bad either. I actually would be happy with anything that fixes this annoying bug. But for both mobile and immobile actors.

Member

MustaphaTR commented Aug 7, 2017

You could block carryall with units in original tho, making carryall take back off(or not land at all if it was already blocked) and go to another cell. I would prefer that behaviour to apply OpenRA too. You could technically do that infinitely and make carryall fail to place its units.

Pchote's idea isn't bad either. I actually would be happy with anything that fixes this annoying bug. But for both mobile and immobile actors.

@abcdefg30 abcdefg30 changed the title from Notify blocking actors from Reinforcements.ReinforceWithTransport to Find alternative landing spots for blocked reinforcements Aug 8, 2017

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Aug 8, 2017

Member

Updated.

Member

abcdefg30 commented Aug 8, 2017

Updated.

@MustaphaTR

This comment has been minimized.

Show comment
Hide comment
@MustaphaTR

MustaphaTR Sep 2, 2017

Member

Needs rebease.

Member

MustaphaTR commented Sep 2, 2017

Needs rebease.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Sep 12, 2017

Member

Rebased.

Member

abcdefg30 commented Sep 12, 2017

Rebased.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 14, 2017

Contributor

OpenRA.Mods.Common/Scripting/Global/ReinforcementsGlobal.cs(160,24): error CS1061: Type OpenRA.Mods.Common.Traits.AircraftInfo' does not contain a definition for CanLand' and no extension method CanLand' of type OpenRA.Mods.Common.Traits.AircraftInfo' could be found. Are you missing an assembly reference?
OpenRA.Mods.Common/Scripting/Global/ReinforcementsGlobal.cs(164,26): error CS1061: Type OpenRA.Mods.Common.Traits.AircraftInfo' does not contain a definition for CanLand' and no extension method CanLand' of type OpenRA.Mods.Common.Traits.AircraftInfo' could be found.

Rebase error?

Contributor

reaperrr commented Sep 14, 2017

OpenRA.Mods.Common/Scripting/Global/ReinforcementsGlobal.cs(160,24): error CS1061: Type OpenRA.Mods.Common.Traits.AircraftInfo' does not contain a definition for CanLand' and no extension method CanLand' of type OpenRA.Mods.Common.Traits.AircraftInfo' could be found. Are you missing an assembly reference?
OpenRA.Mods.Common/Scripting/Global/ReinforcementsGlobal.cs(164,26): error CS1061: Type OpenRA.Mods.Common.Traits.AircraftInfo' does not contain a definition for CanLand' and no extension method CanLand' of type OpenRA.Mods.Common.Traits.AircraftInfo' could be found.

Rebase error?

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Sep 14, 2017

Member

Rebase error?

Yes, fixed. Thanks.

Member

abcdefg30 commented Sep 14, 2017

Rebase error?

Yes, fixed. Thanks.

@RoosterDragon

👍

(Maybe squash the commits? Up to you)

@pchote

LGTM. Verified fix with both Atreides04 and a modified version of nod04b. Just one perf request and then feel free to merge directly.

@abcdefg30 abcdefg30 dismissed stale reviews from pchote and RoosterDragon via 692a026 Oct 7, 2017

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Oct 7, 2017

Member

Updated and rebased.

Just one perf request and then feel free to merge directly.

Can't though as updating dismissed all reviews and you need one review to merge...

Member

abcdefg30 commented Oct 7, 2017

Updated and rebased.

Just one perf request and then feel free to merge directly.

Can't though as updating dismissed all reviews and you need one review to merge...

@reaperrr

Code looks good as far as I can tell and reinforcements on RA shellmap show no regressions.

@reaperrr reaperrr merged commit 2366490 into OpenRA:bleed Oct 8, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr
Contributor

reaperrr commented Oct 8, 2017

@abcdefg30 abcdefg30 deleted the abcdefg30:transBlockers branch Oct 8, 2017

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