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

Allow actors to target terrain without force-fire #21124

Merged
merged 1 commit into from Oct 24, 2023

Conversation

obrakmann
Copy link
Contributor

This is a small addition to AttackBase. It should be useful for things like mortars, artillery or ballistic missiles.

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nits

OpenRA.Mods.Common/Traits/Attack/AttackBase.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Attack/AttackBase.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Attack/AttackBase.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Traits/Attack/AttackBase.cs Outdated Show resolved Hide resolved
@@ -55,6 +55,10 @@ public abstract class AttackBaseInfo : PausableConditionalTraitInfo
[Desc("Tolerance for attack angle. Range [0, 512], 512 covers 360 degrees.")]
public readonly WAngle FacingTolerance = new(512);

[Desc("Show the target cursor even on empty terrain cells when enabled.",
"Any traits implementing IMove must be absent or disabled for this to work.")]
public readonly bool TargetsTerrain = false;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public readonly bool TargetsTerrain = false;
public readonly bool TargetsTerrain = false;

We were discussing on discord that this needs a rename. It's almost sounds as if by having it false the unit won't be able to target terrain at all

Personally I don't have suggestions, sorry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to TargetTerrainWithoutForceFire. Any better?

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

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

LGTM

@PunkPun PunkPun merged commit 4cc9b1b into OpenRA:bleed Oct 24, 2023
3 checks passed
@PunkPun
Copy link
Member

PunkPun commented Oct 24, 2023

Changelog

@obrakmann obrakmann deleted the terrain-targeting branch October 26, 2023 09:47
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

2 participants