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

Refactor handling of hit radii in projectiles. #14742

Merged
merged 1 commit into from Feb 21, 2018

Conversation

Projects
None yet
7 participants
@alercah
Copy link
Contributor

alercah commented Jan 18, 2018

This pull request is intended as a premilinary review of the code changes I'm proposing. I haven't yet done extensive testing or written the necessary upgrade rules. If it does what I expect, it's possible that, as a result of this, shots that weren't previously blocked will now be blocked, but if that's true, it was a bug.

The performance cost from not having a separate radius for blocking actors is easily fixed by storing that on the ActorMap as well, and then having a flag to pass into FindActorsOnLine, if that's a concern.


penev discovered that the RulesetLoaded functions of projectiles were
never being called, meaning that their blocking calculations were not
properly accounting for actors with large hitboxes.

The best fix for this is to change FindActorsOnLine to always account
for the largest actor's hit radius, rather than forcing callers to pass
the largest radius. Per the comment in Util.cs, as a result, move this
computation to ActorMap. I decided to simplify by not making a separate
calculation for actors that block projectiles only; this may cause a
small performance degradation as the search space is a bit larger.

Similarly to this, I've removed the ability to specify a search radius
manually. Because this is only a search radius, setting a value smaller
than the largest eligible actor makes no sense; that would lead to
completely inconsistent blocking. Setting a larger value, on the other
hand, would make no difference.

CreateEffectWarhead was the only place in core code any of these search
radii were set, and that's because 0 was a mysterious magic value that
made the warhead incapable of hitting actors. I replaced it with a
boolean flag that more clearly indicates the actual behaviour.

Fixes #14151.

@abcdefg30 abcdefg30 requested a review from reaperrr Jan 19, 2018

{
[Desc("Size of partition bins (cells)")]
public readonly int BinSize = 10;

// The largest hit radius for any actor.

This comment has been minimized.

@abcdefg30

abcdefg30 Jan 19, 2018

Member

You can include that as Desc. Edit: Wait, actually is this exposed to yaml?

public object Create(ActorInitializer init) { return new ActorMap(init.World, this); }

public void RulesetLoaded(Ruleset rules, ActorInfo info) {
LargestActorRadius = rules.Actors.SelectMany(a => a.Value.TraitInfos<HitShapeInfo>()).Max(h => h.Type.OuterRadius);

This comment has been minimized.

@abcdefg30

abcdefg30 Jan 19, 2018

Member

The brace should be on it's own line. Also spaces -> tabs, please.

/// </summary>
public static IEnumerable<Actor> FindActorsOnCircle(this World world, WPos origin, WDist r)
{
return world.FindActorsInCircle(origin, r + world.ActorMap.LargestActorRadius);

This comment has been minimized.

@abcdefg30

abcdefg30 Jan 19, 2018

Member

Again spaces before return instead of tabs.

{
[Desc("Size of partition bins (cells)")]
public readonly int BinSize = 10;

// The largest hit radius for any actor.
public WDist LargestActorRadius { get; private set; }

This comment has been minimized.

@pchote

pchote Jan 19, 2018

Member

This needs a [FieldLoader.Ignore] and then doesn't need a Desc.

This comment has been minimized.

@alercah

alercah Jan 19, 2018

Author Contributor

That's an error because the attribute isn't valid on properties.

This comment has been minimized.

@pchote

pchote Jan 20, 2018

Member

It might be better to move this to ActorMap to avoid confusion.

This comment has been minimized.

@alercah

alercah Jan 20, 2018

Author Contributor

I was thinking to avoid recalculating it more than once, but since this is a World trait, you're probably right. It's not going to have many instances constructed over the life of the game like another Actor might.

This comment has been minimized.

@alercah

alercah Jan 20, 2018

Author Contributor

Ah wait, but is there a way for the ActorMap to get access to the Ruleset?

This comment has been minimized.

@pchote

pchote Jan 20, 2018

Member

Yes, via the world.Map.

@penev92

This comment has been minimized.

Copy link
Member

penev92 commented Jan 19, 2018

The best fix for this is to change FindActorsOnLine to always account
for the largest actor's hit radius, rather than forcing callers to pass
the largest radius.

Sorry, that doesn't sound right to me. :?
With the callers providing the extra search radius you have more flexibility, while otherwise you have some borderline hardcoded implicit extra value that nobody knows of or expects :S

@alercah

This comment has been minimized.

Copy link
Contributor Author

alercah commented Jan 19, 2018

It's not an extra value. The behaviour of the function as documented is "find all actors whose hit radius intersects a line". It should do that.

The problem is that, for efficiency reasons, the function doesn't want to look at all actors and check their radii. So instead, it looks only a certain distance away from the line. Except it actually doesn't do that, it looks inside a bounding box that contains all points a certain distance away from the line. If this distance is at least as big as the largest actor's radius, it's guaranteed to meet its contract and find all actors.

The code as-is does not do this. It only looks slightly beyond the line. You have to pass it an additional parameter. If the parameter is too small, its actual behaviour is "find all actors that fit inside this kinda arbitrary bounding box that doesn't have much to do with this line and whose hit radius intersect it." If the parameter is too large, it is just a waste of time because it will meet its contract, but it will look at actors it doesn't need to. In other words, there is never any reason for the parameter to be anything other than the exact correct value, and so as a result, it should never be used.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jan 20, 2018

Does this fix #14151?

@alercah

This comment has been minimized.

Copy link
Contributor Author

alercah commented Jan 20, 2018

I've done some rudimentary testing with walls, and it appears they do block projectiles correctly.

@alercah

This comment has been minimized.

Copy link
Contributor Author

alercah commented Jan 20, 2018

I think this is now ready for integration.

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

Looks good to me, although I'§m not sure where we are now with the upgrade rules.

I also wouldn't merge this into the playtest, inb4 actual balance regressions.

👍 nevertheless.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Feb 18, 2018

Haven't tested this yet, but looks good to me.

Since there's currently no telling when/if the current upgrade rule system will be replaced, this PR should get a simple upgrade rule that replaces the VictimScanRadius: 0 entries with ImpactActor: false, though.

@alercah

This comment has been minimized.

Copy link
Contributor Author

alercah commented Feb 18, 2018

Oops, I forgot to push the updated version with an upgrade rule. I pushed it so you can take a look; I'll make sure it rebases cleanly and then repush when I'm done.

@@ -1796,6 +1796,27 @@ internal static void UpgradeWeaponRules(ModData modData, int engineVersion, ref
}
}

if (engineVersion < 20180120)

This comment has been minimized.

@reaperrr

reaperrr Feb 18, 2018

Contributor

Now that the release is out, please change this to a date after the release (exact day doesn't really matter, 20180219 would be enough).

{
node.Value.Nodes.Add(new MiniYamlNode("ImpactActors", "false"));
}
node.Value.Nodes.Remove(victimScanRadius);

This comment has been minimized.

@reaperrr

reaperrr Feb 18, 2018

Contributor

OpenRA.Mods.Common/UtilityCommands/UpgradeRules.cs:L1809: [SA1513] Statements or elements wrapped in curly brackets must be followed by a blank line.

This comment has been minimized.

@MunWolf

MunWolf Feb 18, 2018

Contributor

Single line if statements should not have curly brackets in the first place.

This comment has been minimized.

@reaperrr

reaperrr Feb 18, 2018

Contributor

Ok, but for readability ther should be an empty line between the .Add and the .Remove either way.

This comment has been minimized.

@MunWolf

MunWolf Feb 18, 2018

Contributor

yes :)

if (node.Key.StartsWith("Projectile"))
{
node.Value.Nodes.RemoveAll(n => n.Key == "BounceBlockerScanRadius" || n.Key == "BlockerScanRadius" || n.Key == "AreaVictimScanRadius");
}

This comment has been minimized.

@MunWolf

MunWolf Feb 18, 2018

Contributor

Remove curly brackets

@alercah

This comment has been minimized.

Copy link
Contributor Author

alercah commented Feb 19, 2018

Addressed comments.

@@ -1796,6 +1796,24 @@ internal static void UpgradeWeaponRules(ModData modData, int engineVersion, ref
}
}

if (engineVersion < 20180129)

This comment has been minimized.

@reaperrr

reaperrr Feb 19, 2018

Contributor

typo? should be 0219.

Refactor handling of hit radii in projectiles.
penev discovered that the RulesetLoaded functions of projectiles were
never being called, meaning that their blocking calculations were not
properly accounting for actors with large hitboxes.

The best fix for this is to change FindActorsOnLine to always account
for the largest actor's hit radius, rather than forcing callers to pass
the largest radius. Per the comment in Util.cs, as a result, move this
computation to ActorMap. I decided to simplify by not making a separate
calculation for actors that block projectiles only; this may cause a
small performance degradation as the search space is a bit larger.

Similarly to this, I've removed the ability to specify a search radius
manually. Because this is only a search radius, setting a value smaller
than the largest eligible actor makes no sense; that would lead to
completely inconsistent blocking. Setting a larger value, on the other
hand, would make no difference.

CreateEffectWarhead was the only place in core code any of these search
radii were set, and that's because 0 was a mysterious magic value that
made the warhead incapable of hitting actors. I replaced it with a
boolean flag that more clearly indicates the actual behaviour.

Fixes #14151.
@alercah

This comment has been minimized.

Copy link
Contributor Author

alercah commented Feb 20, 2018

Oops, fixed.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Feb 21, 2018

Does this fix #14151?

Yes. 👍

@reaperrr reaperrr merged commit 08ad7d7 into OpenRA:bleed Feb 21, 2018

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