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

Locomotor, PathGraph, trivial optimizations. #19682

Merged
merged 1 commit into from
Oct 1, 2021

Conversation

anvilvapre
Copy link
Contributor

Do not check for each sub cell if mpos lies in map influence bounds. Save 5 calls for each cell.

Only traverse all bins if there are actors to remove.

if (preferredSubCell != SubCell.Any && !AnyActorsAt(cell, preferredSubCell, checkIfBlocker))
var uv = cell.ToMPos(map);
if (!influence.Contains(uv))
return SubCell.Invalid;
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a behaviour change. Beforehand AnyActorsAt would return false and then map.Grid.DefaultSubCell (or preferredSubCell) would be returned and not SubCell.Invalid?

Copy link
Contributor Author

@anvilvapre anvilvapre Sep 26, 2021

Choose a reason for hiding this comment

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

True. I now realize that Invalid is never the correct return value - but preferredCell or FirstCell.

When a cell is not on the map - a sub cell is returned - either the preferred or the first. The caller will not know it passed an off map cell. It might be better to return Invalid perhaps to allow the caller to cancel further processing. I.e. path planning may now continue longer than needed?

In the last push original behavior is maintained.

@RoosterDragon
Copy link
Member

LGTM modulo the comment r.e. the change in behaviour.

RoosterDragon
RoosterDragon previously approved these changes Sep 27, 2021
@@ -272,9 +272,10 @@ public IEnumerable<Actor> GetActorsAt(CPos a, SubCell sub)
if (!influence.Contains(uv))
yield break;

var always = (sub == SubCell.FullCell || sub == SubCell.Any);
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for the parentheses.

Copy link
Member

@abcdefg30 abcdefg30 left a comment

Choose a reason for hiding this comment

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

Changes lgtm and didn't spot issues on a cursory test.

@abcdefg30 abcdefg30 merged commit 9d4d4bb into OpenRA:bleed Oct 1, 2021
@abcdefg30
Copy link
Member

Changelog

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

Successfully merging this pull request may close these issues.

None yet

4 participants