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

Merge HeliLand into Land #16365

Merged
merged 3 commits into from May 17, 2019

Conversation

@reaperrr
Copy link
Contributor

commented Mar 30, 2019

Another part of #15803 that could be split off relatively easy because there aren't any real dependencies (although I made it depend on #16241 to keep the rebase diff small).

@reaperrr reaperrr force-pushed the reaperrr:merge-Land branch from 3a9945e to 29ec422 Mar 30, 2019

@matjaeck

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

Needs a rebase.

@reaperrr reaperrr referenced this pull request Apr 11, 2019
0 of 1 task complete

@reaperrr reaperrr force-pushed the reaperrr:merge-Land branch from 29ec422 to 1f60b0b Apr 22, 2019

@@ -301,14 +301,14 @@ protected virtual void Tick(Actor self)

// Add land activity if LandOnCondidion resolves to true and the actor can land at the current location.
if (!ForceLanding && landNow.HasValue && landNow.Value && airborne && CanLand(self.Location)
&& !(self.CurrentActivity is HeliLand || self.CurrentActivity is Turn))
&& !((self.CurrentActivity is Land && Info.VTOL) || self.CurrentActivity is Turn))

This comment has been minimized.

Copy link
@tovl

tovl Apr 22, 2019

Contributor

Does this have to be exclusive to VTOL craft?

This comment has been minimized.

Copy link
@reaperrr

reaperrr Apr 25, 2019

Author Contributor

It doesn't have to, but might not work well with non-VTOLs. I removed the limitation, but fixing any bugs with non-VTOLs would likely require refactoring Land further to handle the case of "already at target location, but up in the air" for non-VTOLs, which I feel is a bit out of scope for this PR.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

Updated.

@pchote
Copy link
Member

left a comment

Unfortunately this regresses TS carryalls quite badly.

  • Carryalls can't pickup actors (fix noted below)
  • Carryall "lands" at ground level after picking up the actor, pushing the cargo below ground level
  • Carryall dances around instead of landing to drop the cargo (maybe related to my landing position question above?)
OpenRA.Mods.Common/Activities/PickupUnit.cs Outdated Show resolved Hide resolved
@tovl

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2019

Carryall "lands" at ground level after picking up the actor, pushing the cargo below ground level

This is already true on bleed and will be fixed by #16417.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

Carryalls can't pickup actors (fix noted below)

Fixed (locally).

Carryall "lands" at ground level after picking up the actor, pushing the cargo below ground level

As @tovl said.

Carryall dances around instead of landing to drop the cargo (maybe related to my landing position question above?)

This one I have no clue how to fix. All I can tell is that the Carryall never reaches Wait or Release state, so it seems somehow if (self.World.Map.DistanceAboveTerrain(carryablePosition) != WDist.Zero) always resolves to true for some reason, or that the Land activity itself makes the carryall take off again before we can trigger Release.
Could also be that inaccurate landing offset + AltitudeVelocity make it always overshoot WDist.Zero, but changing the check to > WDist.Zero didn't fix it, so that's unlikely.
In any case, I'll need help debugging this if we want this PR merged soon, I have trouble getting my head around it right now.

@pchote pchote added this to the Next Release milestone Apr 28, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

Wrapping up these aircraft PRs is one of my priorities for the milestone, so i'll see what I can do over the next few days.

@reaperrr reaperrr force-pushed the reaperrr:merge-Land branch from 16baa04 to 70eb104 May 12, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

Rebased, applied @tovl's fixes from #16509 (squashed into 2nd commit), applied GroundPosition change request (also squashed into 2nd commit).

OpenRA.Mods.Common/Activities/Air/Land.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Activities/Air/Land.cs Outdated Show resolved Hide resolved

@reaperrr reaperrr force-pushed the reaperrr:merge-Land branch from 70eb104 to 5450b38 May 14, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

Updated.

@pchote

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Activities/Air/Land.cs(104,40): error CS1503: Argument 3: cannot convert from 'OpenRA.WDist' to 'int' [/home/travis/build/OpenRA/OpenRA/OpenRA.Mods.Common/OpenRA.Mods.Common.csproj]

@reaperrr reaperrr force-pushed the reaperrr:merge-Land branch from 5450b38 to 5ce3506 May 15, 2019

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Fixed.

OpenRA.Mods.Common/Activities/Air/Land.cs Outdated Show resolved Hide resolved
else
QueueChild(self, new Land(self, Target.FromPos(dest.CenterPosition + offset), true, dest), true);
if (aircraft.Info.VTOL && aircraft.Info.TurnToDock)
QueueChild(self, new Turn(self, aircraft.Info.InitialFacing), true);

This comment has been minimized.

Copy link
@pchote

pchote May 15, 2019

Member

These Turns are logically part of the landing behaviour, so really belong inside the Land activity.

This is already done in #16509, so we don't need to address this here.

@pchote

This comment has been minimized.

Copy link
Member

commented May 15, 2019

LGTM otherwise, so I expect to 👍 after my question above is resolved.

@reaperrr reaperrr force-pushed the reaperrr:merge-Land branch from 5ce3506 to 00d8fc8 May 15, 2019

reaperrr added 2 commits Apr 25, 2019
Streamline Land activity
Removed some redundant parameters, some redundant overloads
and made Land always consider LandAltitude relative to target.
Allow non-VTOL to LandOnCondition
Note: This might still break in unexpected ways,
since non-VTOLs implicitly rely on ReturnToBase
to calculate the approach vector for them.

@reaperrr reaperrr force-pushed the reaperrr:merge-Land branch from 00d8fc8 to 94b2ab8 May 15, 2019

return NextActivity;

if (IsCanceling)
if (IsCanceling || target.Type == TargetType.Invalid)
{
aircraft.RemoveInfluence();
return NextActivity;

This comment has been minimized.

Copy link
@pchote

pchote May 15, 2019

Member

This returns without putting the actor back into a consistent state - stopping the helicopter or losing the target while the land is half done will leave the helicopter sitting at an intermediate altitude.

This is already the case on bleed and I think may already be solved in #16509, so we don't need to address this here.

@pchote
pchote approved these changes May 15, 2019

@pchote pchote added the PR: Needs +2 label May 15, 2019

@tovl

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

LGTM 👍

@pchote pchote merged commit 7becbe6 into OpenRA:bleed May 17, 2019

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
5 participants
You can’t perform that action at this time.