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

Warhead refactor part 3: Add WarheadArgs #17119

Merged
merged 1 commit into from Jan 21, 2020
Merged

Conversation

@reaperrr
Copy link
Contributor

reaperrr commented Sep 18, 2019

Adds WarheadArgs to simplify warhead code and make it easier to add more arguments in the future without having to update the warheads and their methods each time.

On the code side, the main concern is whether the initial set of args is sufficient and conceptually sound.
On the testing side, you only need to check whether this causes any regressions (which it shouldn't, but you never know).

@reaperrr reaperrr added this to the Next+1 milestone Sep 18, 2019
@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Sep 19, 2019

Something I would propose to a followup: the last owner of the firer, to fix that issue when weapons fired by already-dead units explode without an owner.

@reaperrr reaperrr force-pushed the reaperrr:wh-refactor3 branch from 260dba7 to 41b285d Sep 26, 2019
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Sep 26, 2019

Updated. Streamlined this a bit and removed LastProjectileFacing, but don't worry @GraionDilach, I have another follow-up that actually takes things one step further, using WRot for better forward-compatibility with the int-to-WAngle migration Facing will eventually get, as well as Pitch support (vertical impact angle).
This should make full directional armor support (front, rear, sides, top and bottom) much easier to implement.

@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Dec 30, 2019

Rebased and decoupled from #17014.

@pchote This PR and #17280 are the ones I actually want most urgently (for my downstream directional armor implementation, mostly), so I'd prefer if you focused on this and #17280 over further discussion/review of #17014 for now.

Copy link
Member

pchote left a comment

Good idea, and a long time coming.

A couple of suggestions to reduce duplication and maintenance effort if we do need to add new fields in the future:

OpenRA.Game/GameRules/WeaponInfo.cs Outdated Show resolved Hide resolved
OpenRA.Game/GameRules/WeaponInfo.cs Show resolved Hide resolved
@@ -231,7 +232,17 @@ public void Tick(World world)
foreach (var a in actors)
{
var adjustedModifiers = args.DamageModifiers.Append(GetFalloff((args.Source - a.CenterPosition).Length));

This comment has been minimized.

Copy link
@reaperrr

reaperrr Jan 9, 2020

Author Contributor

Unrelated to this PR, but... am I reading this wrong, or does damage fall off the farther the hit actor is from the source actor, instead of the farther it is from the beam's center (which would be the correct behavior)?

This comment has been minimized.

Copy link
@pchote

pchote Jan 11, 2020

Member

It looks like the desired behaviour according to #9357 was to have a certain section of the beam between headPos and x that deals maximum damage, then a linear falloff in damage between x and y, and then the remaining section from y to tailPos deal minimum damage.

@reaperrr reaperrr force-pushed the reaperrr:wh-refactor3 branch from cae98b7 to 4f6f9fa Jan 9, 2020
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Jan 9, 2020

Updated.

Copy link
Member

pchote left a comment

A couple of minor thoughts, otherwise LGTM. I haven't tested this yet ingame.

OpenRA.Game/GameRules/WeaponInfo.cs Outdated Show resolved Hide resolved
OpenRA.Game/GameRules/WeaponInfo.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Cnc/Projectiles/IonCannon.cs Show resolved Hide resolved
Copy link
Member

pchote left a comment

Found a couple of crashes while testing:

OpenRA.Game/GameRules/WeaponInfo.cs Outdated Show resolved Hide resolved
OpenRA.Mods.Common/Projectiles/NukeLaunch.cs Outdated Show resolved Hide resolved
- Passes additional arguments to warheads on impact
- Uses that to reduce parameter count of DoImpact by 1
@reaperrr reaperrr force-pushed the reaperrr:wh-refactor3 branch from 4f6f9fa to 4ae791b Jan 16, 2020
@reaperrr

This comment has been minimized.

Copy link
Contributor Author

reaperrr commented Jan 16, 2020

Updated.

@pchote
pchote approved these changes Jan 18, 2020
@pchote pchote added the PR: Needs +2 label Jan 18, 2020
@teinarss teinarss merged commit 6220d7e into OpenRA:bleed Jan 21, 2020
2 checks passed
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
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.