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

Rewrite Leap and AttackLeap, add LeapAttack and EdibleByAttackLeap #15008

Merged
merged 3 commits into from Dec 29, 2018

Conversation

Projects
None yet
6 participants
@abcdefg30
Copy link
Member

abcdefg30 commented Mar 30, 2018

Yet another attempt at resolving the AttackLeap mess. Supersedes #13513.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Mar 30, 2018

Something I didn't understand back at #13513 either... if you remove the attacking angle, why are you still calling this a leap? This becomes a jousting attack then.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Mar 30, 2018

AttackPoke? 😄

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Mar 30, 2018

Well, there will be need for a variant with the angle being kept for RA2 atleast longterm which should have the AttackLeap name, so I'm fine with AttackPoke/AttackJoust/etc.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Apr 10, 2018

I'll go with AttackJoust then if noone objects.

@Smittytron

This comment has been minimized.

Copy link
Contributor

Smittytron commented Apr 18, 2018

Played around with this. Naming conventions aside, this is fixes the dog issues I know of. I doubt we'll see unwelcome balance implications here but that's something we can address later as needed.

@UsernameNotSure

This comment has been minimized.

Copy link

UsernameNotSure commented Apr 21, 2018

Hi, just registered to outline the issues on this again.
The dog leap logic has many individual factors that you most likely DON'T want to put into a generalized, single design.

The issues:

  1. Pathfinding yes / no:
    If the leap range is a huge amount, let's say 9 fields, dogs would be able to jump through structures, over sandbags / walls, over rivers, up / down cliffs -> line of fire check for the weapon in terms of passable terrain to (dis-)allow such jumps
  2. Instakill yes / no:
    Normally dogs should instakill other dogs and infantry. Eventually that'll change one day. So it should be possible to define whether to use the dog's "weapon damage" or a plain instakill. Eventually also with individual damage tables vs certain units, such as Tanya to get jumped but not killed instantly (and such actions would then instead result in the dog dealing damage to the target unit but being killed by it).
  3. two dogs jumping each other simultaneously:
    Before both ended up in an endless loop of jumping another. Workarounds could be to randomly assign one dog that gets jumped and the other one doing the jump. As both should die anyway a new animation would be required in which the dogs kill another - unless the in "2)" desribed damage table was used in which case the jumped would receive damage but end up killing the jumping unit.
  4. many dogs, same target:
    If multiple dogs are assigned the same target their unit-target order should become a ground-target order once the 1st dog leaps at the target and in general any target that's dog-leaped shouldn't be targeted by other dogs anymore. Otherwise it'd be possible that many dogs pile up on the same x,y coords, standing inside another or eventually kill each other if their projectile may cause that.
@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Apr 24, 2018

  1. is out of scope for this PR.
  2. - 4. are solved in this PR, dogs deal the damage defined on the warhead, don't loop endlessly and also don't pile up on the same targets location.
@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented May 4, 2018

@abcdefg30

and after looking the term up, I'm not even sure how that is an accurate description of what the former AttackLeap is doing

It's not accurate to the former setup and that is part of my reasoning. It'd be more accurate for this version though. The thing is, both this PR and the predecessor axes the WPos.LerpQuadratic - which has an angle - in favor of WPos.Lerp - which lacks the angle. Without said angle, there isn't any real leap either.

Sure, joust might be a bad idea, since I'd be using it more of it's archaic Latin-based "to approach, to meet" meaning than the knight's sport, but tbh that is the most generic term I could've come up with which somewhat applies to any potential usecase of charge-up-trample/joust/bite kind of attack (since charging is already used for powered-up firing) logic.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 4, 2018

My serious suggestion on IRC was AttackBite. It is generic enough while still being understandable.

@Smittytron

This comment has been minimized.

Copy link
Contributor

Smittytron commented May 4, 2018

AttackLunge?

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented May 4, 2018

Again, I am fine with anything as long as it's not kept as Leap, since there is no leap anymore but this old AttackLeap will have to be hacked into the RA2 repo, following the bag handling's footsteps and it feels weird to come up with a name for the old logic because the new logic has the old name as an artifact.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 7, 2018

It would be good to get this in for the playtest, so can you proceed with the rename @abcdefg30? This PR is breaking compatibility with the old trait behaviour, so we will need some form of update rule too. I suggest (assuming the rename goes ahead) automatically removing the AttackLeap trait and giving a manual step saying that that trait has been removed, and to define new attack logic using the AttackBite (or whatever) trait.

Show resolved Hide resolved mods/ra/rules/infantry.yaml Outdated
Show resolved Hide resolved mods/ra/rules/infantry.yaml Outdated

@pchote pchote added this to the Next release milestone May 21, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 21, 2018

It looks like it might make sense to rebase this on top of #15140?

We may be able to use GrantConditionOnArmamentReloading to grant the rip condition and the INotifyAiming interface (via a new GrantConditionWhileAiming trait) to grant the run condition?

This could simplify things here a lot.

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented May 22, 2018

Makes sense, let's try that.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jun 3, 2018

#15140 has now been merged.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Jun 22, 2018

It sounds like @abcdefg30 won't have time to work on this again soon, so dropping from the milestone so we can focus on the other areas.

@pchote pchote removed this from the Next release milestone Jun 22, 2018

@abcdefg30 abcdefg30 force-pushed the abcdefg30:attackDogs branch from 5bedef0 to 4ef7d3f Oct 10, 2018

Show resolved Hide resolved mods/ra/rules/infantry.yaml Outdated
Show resolved Hide resolved mods/ra/rules/infantry.yaml Outdated

@pchote pchote added this to the Next Release milestone Dec 26, 2018

@pchote
Copy link
Member

pchote left a comment

Just a couple of comments/questions, otherwise LGTM. Seems to behave well ingame.

Show resolved Hide resolved OpenRA.Mods.Cnc/Activities/LeapAttack.cs Outdated
Show resolved Hide resolved OpenRA.Mods.Cnc/Activities/LeapAttack.cs Outdated
Show resolved Hide resolved OpenRA.Mods.Cnc/Traits/EdibleByLeap.cs Outdated
@Smittytron
Copy link
Contributor

Smittytron left a comment

Tested this with dog-heavy missions like Soviet 03 and Soviet 07. I don't think we'll need to re-balance anything.
New dogs look great.

@abcdefg30 abcdefg30 force-pushed the abcdefg30:attackDogs branch from 579e8b9 to f5daa6a Dec 29, 2018

@abcdefg30

This comment has been minimized.

Copy link
Member

abcdefg30 commented Dec 29, 2018

Updated.

@pchote pchote added the PR: Needs +2 label Dec 29, 2018

@pchote pchote requested a review from reaperrr Dec 29, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Dec 29, 2018

@reaperrr is this going to conflict with #15675 too much?

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Dec 29, 2018

@reaperrr is this going to conflict with #15675 too much?

There's going to be a lot of merge conflicts, but one of our PRs was bound to have to go through that. Not worth delaying this PR over.

@reaperrr reaperrr force-pushed the abcdefg30:attackDogs branch from f5daa6a to 2b948c3 Dec 29, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Dec 29, 2018

👍
Only two tiny issues found: LeapAttack was in Common namespace instead of Cnc, and the last commits' title was a bit too long.
I didn't want to delay the merge over such minor issues, so I pushed the fixes myself and will merge this after Travis approves.

@reaperrr reaperrr force-pushed the abcdefg30:attackDogs branch from 2b948c3 to a160d8a Dec 29, 2018

@reaperrr reaperrr merged commit 0ff4e46 into OpenRA:bleed Dec 29, 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