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

Land activity: fix bug which causes crash in Aircraft.AddInflunce() #21346

Merged
merged 1 commit into from Feb 19, 2024

Conversation

michaeldgg2
Copy link
Contributor

This PR fixes #21302 by adding additional check in Land.Tick() for skipping landing mechanism, if the aircraft is already on the ground at target location.

More details

Land activity has an early check, if the aircraft is at the landing location:

var pos = aircraft.GetPosition();

// Reevaluate target position in case the target has moved.
targetPosition = target.CenterPosition + offset;
landingCell = self.World.Map.CellContaining(targetPosition);

// We are already at the landing location.
if ((targetPosition - pos).LengthSquared == 0)
    return true;

However, this can change, since the desired landing location can be occupied:

var newLocation = aircraft.FindLandingLocation(landingCell, landRange);

// ...

if (newLocation.Value != landingCell)
{
    target = Target.FromCell(self.World, newLocation.Value);
    targetPosition = target.CenterPosition + offset;
    landingCell = self.World.Map.CellContaining(targetPosition);
}

After new landing location is picked, the check isn't performed again and Aircraft.AddInfluence() can be called without calling Aircraft.RemoveInfluence() first.

This can happen, if the aircraft is on the ground and force land order is issued to the adjacent cell, which is occupied (doesn't matter what kind of actor is on that cell). Since the aircraft does not take off first, when forcing the landing, but immediately attempts to land on the adjacent cell, it does not do the check mentioned above.

It is possible that an older issue #20459 is caused by the same bug as the crash described in #21302.

@darkademic
Copy link
Contributor

Can confirm that it fixes the crash, and the explanation/fix makes sense to me.

@PunkPun PunkPun added this to the Next Release milestone Feb 18, 2024
Copy link
Member

@atlimit8 atlimit8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed and tested. It fixes the adjacent force landing crash mentioned by @darkademic. Aborting here is the correct behavior regardless of the bug.
I have one nit about the comment wording; otherwise, I approve. 👍

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

@atlimit8 atlimit8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor copy-pasta on the comment change.

@PunkPun PunkPun merged commit 3760b14 into OpenRA:bleed Feb 19, 2024
3 checks passed
@PunkPun
Copy link
Member

PunkPun commented Feb 19, 2024

changelog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot AddInfluence until previous influence is removed with RemoveInfluence
4 participants