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

Some work on the missile projectile #8717

Merged
merged 4 commits into from Oct 13, 2015

Conversation

Projects
None yet
@matija-hustic
Contributor

matija-hustic commented Jul 15, 2015

An attempt at tackling #7509.

Introduces:

  • vertical rate of turn
  • customizable vertical launch angle
  • delayed activation of the homing mechanism / propulsion
  • freefall while propulsion deactivated
  • customizable explosion altitude (for airburst)
  • customizable cruise altitude (when target out of range)
  • height checks for terrain impact and shadow rendering
  • acceleration (instead of constant speed from launch to impact)

Not sure about what was meant in #7509 by

  • proper 3D math

Still missing and out of scope of this PR (at least currently)

  • support for voxel artwork

@matija-hustic matija-hustic changed the title from Missile effect to Some work on the missile effect Jul 15, 2015

@matija-hustic matija-hustic changed the title from Some work on the missile effect to Some work on the missile projectile Jul 15, 2015

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Jul 15, 2015

Contributor

acceleration (instead of constant speed from launch to impact)

Starting with low speed and gradually become faster, rather than flying at constant speed.

Contributor

reaperrr commented Jul 15, 2015

acceleration (instead of constant speed from launch to impact)

Starting with low speed and gradually become faster, rather than flying at constant speed.

@matija-hustic

This comment has been minimized.

Show comment
Hide comment
@matija-hustic

matija-hustic Jul 15, 2015

Contributor

I'll play around with the acceleration next. I guess that that would mean

  • an additional upper speed limit will be needed
  • too high speeds might adversely affect accuracy
  • testing how it'd work with the freefall mode
Contributor

matija-hustic commented Jul 15, 2015

I'll play around with the acceleration next. I guess that that would mean

  • an additional upper speed limit will be needed
  • too high speeds might adversely affect accuracy
  • testing how it'd work with the freefall mode
@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Jul 16, 2015

Contributor

Sorry, I took only a quick glampse so far.

What is freefall mode you refer to? Does that apply to missiles which already passed their RangeLimit but has ExplodeWhenEmpty=false and refers that they fall to the ground without doing(?) damage?

Does ActivationDelay means RA2 CourseLockDuration - missile is forced into the initial facing for x frames and only then starts to guide itself to target?

Contributor

GraionDilach commented Jul 16, 2015

Sorry, I took only a quick glampse so far.

What is freefall mode you refer to? Does that apply to missiles which already passed their RangeLimit but has ExplodeWhenEmpty=false and refers that they fall to the ground without doing(?) damage?

Does ActivationDelay means RA2 CourseLockDuration - missile is forced into the initial facing for x frames and only then starts to guide itself to target?

@matija-hustic

This comment has been minimized.

Show comment
Hide comment
@matija-hustic

matija-hustic Jul 22, 2015

Contributor

@GraionDilach, you have understood the RangeLimit and ExplodeWhenEmpty == false correctly. But the missile explodes when it touches ground ultimately.

The ActivationDelay also uses freefall (not constant velocity) before the guiding starts. This means that the missile does not just travel in a given direction, but that it follows a ballistic trajectory. You would want to use the LaunchAngle and InitialSpeed here.

Contributor

matija-hustic commented Jul 22, 2015

@GraionDilach, you have understood the RangeLimit and ExplodeWhenEmpty == false correctly. But the missile explodes when it touches ground ultimately.

The ActivationDelay also uses freefall (not constant velocity) before the guiding starts. This means that the missile does not just travel in a given direction, but that it follows a ballistic trajectory. You would want to use the LaunchAngle and InitialSpeed here.

@matija-hustic

This comment has been minimized.

Show comment
Hide comment
@matija-hustic

matija-hustic Aug 4, 2015

Contributor

OK, updated. The TODO comments will be taken care of when the YAML upgrade rule is in place.

Contributor

matija-hustic commented Aug 4, 2015

OK, updated. The TODO comments will be taken care of when the YAML upgrade rule is in place.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Aug 4, 2015

Member

OpenRA.Mods.Common/Effects/Missile.cs(382,36): error CS0103: The name "shadowPos" does not exist in the current context

Member

abcdefg30 commented Aug 4, 2015

OpenRA.Mods.Common/Effects/Missile.cs(382,36): error CS0103: The name "shadowPos" does not exist in the current context

@matija-hustic

This comment has been minimized.

Show comment
Hide comment
@matija-hustic

matija-hustic Aug 4, 2015

Contributor

I've removed the missile shadow part cause it's done in #8923

Contributor

matija-hustic commented Aug 4, 2015

I've removed the missile shadow part cause it's done in #8923

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 4, 2015

Contributor

Gave this some ingame testing, works nice for the most part!

One thing I noticed, though: increasing the AirburstAltitude does not only increase the altitude above, but also distance from target the explosion will happen. With a TS Nod bazooka, an AirburstAltitude value of 1024 made the missile explode after little more than half the maximum distance (and RangeLimit was disabled, so that can't be the problem).

👍 from me after that is adressed.

Contributor

reaperrr commented Aug 4, 2015

Gave this some ingame testing, works nice for the most part!

One thing I noticed, though: increasing the AirburstAltitude does not only increase the altitude above, but also distance from target the explosion will happen. With a TS Nod bazooka, an AirburstAltitude value of 1024 made the missile explode after little more than half the maximum distance (and RangeLimit was disabled, so that can't be the problem).

👍 from me after that is adressed.

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Aug 20, 2015

Member

Bump.
@matija-hustic you still around?

Member

penev92 commented Aug 20, 2015

Bump.
@matija-hustic you still around?

@matija-hustic

This comment has been minimized.

Show comment
Hide comment
@matija-hustic

matija-hustic Aug 20, 2015

Contributor

@Penev Yep, just without my computer for another ten days. I'll get to the
PRs as soon as I can. But all of the requested changes are minor if I
recall correctly
Em 20/08/2015 10:53, "Pavel Penev" notifications@github.com escreveu:

Bump.
@matija-hustic https://github.com/matija-hustic you still around?


Reply to this email directly or view it on GitHub
#8717 (comment).

Contributor

matija-hustic commented Aug 20, 2015

@Penev Yep, just without my computer for another ten days. I'll get to the
PRs as soon as I can. But all of the requested changes are minor if I
recall correctly
Em 20/08/2015 10:53, "Pavel Penev" notifications@github.com escreveu:

Bump.
@matija-hustic https://github.com/matija-hustic you still around?


Reply to this email directly or view it on GitHub
#8717 (comment).

@Unit158

This comment has been minimized.

Show comment
Hide comment
@Unit158

Unit158 Sep 4, 2015

Contributor

Any progress?

Contributor

Unit158 commented Sep 4, 2015

Any progress?

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 6, 2015

Contributor

@matija-hustic if you're still around, can you give an estimation when you'll be able to update and rebase this?

Contributor

reaperrr commented Sep 6, 2015

@matija-hustic if you're still around, can you give an estimation when you'll be able to update and rebase this?

@matija-hustic

This comment has been minimized.

Show comment
Hide comment
@matija-hustic

matija-hustic Sep 10, 2015

Contributor

OK, so I've updated this. Hopefully the airburst works a little better now?
@reaperrr, I may not understand 100% what airburst should do. Without airburst, the missile is supposed to explode when inside a ball of radius CloseEnough arond the target. With airburst, I've made the missle explode when inside a cylinder of radius CloseEnough and height AirburstAltitude. Is this approximately what should happen?

Previously airburst involved only height and velocity (to explode only when on the downward part of the trajectory) checks, no distance checks

Contributor

matija-hustic commented Sep 10, 2015

OK, so I've updated this. Hopefully the airburst works a little better now?
@reaperrr, I may not understand 100% what airburst should do. Without airburst, the missile is supposed to explode when inside a ball of radius CloseEnough arond the target. With airburst, I've made the missle explode when inside a cylinder of radius CloseEnough and height AirburstAltitude. Is this approximately what should happen?

Previously airburst involved only height and velocity (to explode only when on the downward part of the trajectory) checks, no distance checks

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Sep 10, 2015

Contributor

Is this approximately what should happen?

@matija-hustic That's the right direction, but not quite there yet: In TS, airburst missiles would not descent towards the target, instead they'd fly towards and (visually at least, the damage was still dealt to the ground but that's out of this PR's scope) explode at targetpos.Z + AirburstAltitude (original TS used some magic hardcoded number of cells above ground, I think).

That being said, I digged in my memory and the only Airburst weapon in TS without the Firestorm addon is the voxel cluster missile, so this feature is just a nice-to-have and far from urgent for sprite missiles, at least for now. If you feel like it, you can try to reproduce the behavior described above of course, but maybe it would be better to split Airburst off to its own PR depending on this one, so this PR won't get blocked over any airburst-related issues (I suspect the warheads might need some fiddling as well to reproduce the behavior of the original).

Contributor

reaperrr commented Sep 10, 2015

Is this approximately what should happen?

@matija-hustic That's the right direction, but not quite there yet: In TS, airburst missiles would not descent towards the target, instead they'd fly towards and (visually at least, the damage was still dealt to the ground but that's out of this PR's scope) explode at targetpos.Z + AirburstAltitude (original TS used some magic hardcoded number of cells above ground, I think).

That being said, I digged in my memory and the only Airburst weapon in TS without the Firestorm addon is the voxel cluster missile, so this feature is just a nice-to-have and far from urgent for sprite missiles, at least for now. If you feel like it, you can try to reproduce the behavior described above of course, but maybe it would be better to split Airburst off to its own PR depending on this one, so this PR won't get blocked over any airburst-related issues (I suspect the warheads might need some fiddling as well to reproduce the behavior of the original).

@matija-hustic

This comment has been minimized.

Show comment
Hide comment
@matija-hustic

matija-hustic Sep 10, 2015

Contributor

Ah OK! That should then be very easy to do. I'll get to it today.

Contributor

matija-hustic commented Sep 10, 2015

Ah OK! That should then be very easy to do. I'll get to it today.

Show outdated Hide outdated OpenRA.Game/WAngle.cs
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 7, 2015

Member

This looks pretty good ingame (based on very limited testing). One suggestion, which can be left to a later PR if you don't want to do it now, is to account for the vertical angle in the sprite facing calculation like we do for bullets so that the missile image is aligned with the apparent direction of travel instead of the horizontal direction.

Member

pchote commented Oct 7, 2015

This looks pretty good ingame (based on very limited testing). One suggestion, which can be left to a later PR if you don't want to do it now, is to account for the vertical angle in the sprite facing calculation like we do for bullets so that the missile image is aligned with the apparent direction of travel instead of the horizontal direction.

@matija-hustic

This comment has been minimized.

Show comment
Hide comment
@matija-hustic

matija-hustic Oct 7, 2015

Contributor

The code is a little cleaner now. There are a couple of remaining TODOs.
I need some help with the following:

  • How do I decide which variables should have [Sync] next to them?

The other ones include

  • possibly unhardcoding some values from the homing code,
  • dynamically selected initial speed and launch angle (better interaction of units firing near cliffs),
  • tweaking the missile's rendering to account for the vertical angle.

These can be left for later if there's a hurry to get the PR off the queue.

Contributor

matija-hustic commented Oct 7, 2015

The code is a little cleaner now. There are a couple of remaining TODOs.
I need some help with the following:

  • How do I decide which variables should have [Sync] next to them?

The other ones include

  • possibly unhardcoding some values from the homing code,
  • dynamically selected initial speed and launch angle (better interaction of units firing near cliffs),
  • tweaking the missile's rendering to account for the vertical angle.

These can be left for later if there's a hurry to get the PR off the queue.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Oct 7, 2015

Member

How do I decide which variables should have [Sync] next to them?

As few as you can get away with while still catching any desyncs early. The position and facing are probably sufficient.

How do I get an integer square root of an integer (deterministically)?

Exts.ISqrt

Member

pchote commented Oct 7, 2015

How do I decide which variables should have [Sync] next to them?

As few as you can get away with while still catching any desyncs early. The position and facing are probably sufficient.

How do I get an integer square root of an integer (deterministically)?

Exts.ISqrt

@matija-hustic

This comment has been minimized.

Show comment
Hide comment
@matija-hustic

matija-hustic Oct 7, 2015

Contributor

The position and facing are probably sufficient.

So I can remove [Sync] from the other ones (e.g. targetPosition, offset, etc. that were there before I modified Missile.cs)?

Contributor

matija-hustic commented Oct 7, 2015

The position and facing are probably sufficient.

So I can remove [Sync] from the other ones (e.g. targetPosition, offset, etc. that were there before I modified Missile.cs)?

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Oct 9, 2015

Member

I see this was silently updated.

Member

penev92 commented Oct 9, 2015

I see this was silently updated.

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Oct 10, 2015

Contributor

Alright, I won't try to understand all the logic behind the calculations in this PR, but it certainly looks good in-game. The cliff avoidance in TS works, and the missiles in RA still do what they're supposed to do (jammers work, ballistic missiles from subs still work, the rests, too).

Still, there are two issues (well, probably just one): the missiles from the Nod bike and stealth tank are invisible when the target is below or at the same height as the unit. They do show up when the target is higher. That should be fixed, then it gets my 👍 as well.

Contributor

obrakmann commented Oct 10, 2015

Alright, I won't try to understand all the logic behind the calculations in this PR, but it certainly looks good in-game. The cliff avoidance in TS works, and the missiles in RA still do what they're supposed to do (jammers work, ballistic missiles from subs still work, the rests, too).

Still, there are two issues (well, probably just one): the missiles from the Nod bike and stealth tank are invisible when the target is below or at the same height as the unit. They do show up when the target is higher. That should be fixed, then it gets my 👍 as well.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Oct 11, 2015

Contributor

Needs rebase as well.

Contributor

reaperrr commented Oct 11, 2015

Needs rebase as well.

matija-hustic added some commits Jul 14, 2015

Missile projectile refactor.
Introduces:

Vertical rate of turn
Customizable vertical launch angle
Delayed activation of the homing mechanism / propulsion
Freefall while propulsion deactivated
Customizable explosion altitude (for airburst)
Customizable cruise altitude (when target out of range)
Height checks for terrain impact and shadow rendering
Acceleration (instead of constant speed from launch to impact)
Added lookahead, launch speed & angle computation.
The missiles should be more intelligent, avoiding cliffs,
surmounting inclines and flexibly selecting appropriate
launch speed and angle to avoid a close incline or miss
a close target.
Downward lookahead.
Added downward lookahead capability.
@matija-hustic

This comment has been minimized.

Show comment
Hide comment
@matija-hustic

matija-hustic Oct 11, 2015

Contributor

Making this thing know how to avoid obstacles is proving to be a handful. As it turns out, the previous version of the lookahead didn't handle firing downhill properly...
Now it should finally handle both uphill and downhill firing with some intelligence. But I'll need some more time for testing and going through the code.

@obrakmann, the bike and stealth tank should now be able to fire normally. The previous bug was a consequence of incorrectly choosing negative launch angles.

Contributor

matija-hustic commented Oct 11, 2015

Making this thing know how to avoid obstacles is proving to be a handful. As it turns out, the previous version of the lookahead didn't handle firing downhill properly...
Now it should finally handle both uphill and downhill firing with some intelligence. But I'll need some more time for testing and going through the code.

@obrakmann, the bike and stealth tank should now be able to fire normally. The previous bug was a consequence of incorrectly choosing negative launch angles.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Oct 11, 2015

Contributor

Retested since there have been a few updates since I last looked at this.

The look-ahead works fine now, the vertical angle is factored into the sprite facing and the rest still works. So I'm renewing my 👍

Contributor

reaperrr commented Oct 11, 2015

Retested since there have been a few updates since I last looked at this.

The look-ahead works fine now, the vertical angle is factored into the sprite facing and the rest still works. So I'm renewing my 👍

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Oct 13, 2015

Contributor

Yes, works fine now, 👍

Contributor

obrakmann commented Oct 13, 2015

Yes, works fine now, 👍

obrakmann added a commit that referenced this pull request Oct 13, 2015

Merge pull request #8717 from matija-hustic/missile
Some work on the missile projectile

@obrakmann obrakmann merged commit 410f121 into OpenRA:bleed Oct 13, 2015

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matija-hustic

This comment has been minimized.

Show comment
Hide comment
@matija-hustic

matija-hustic Oct 13, 2015

Contributor

I wanted to clean up the code some more. And make sure no other problems appear... Are you sure you want to merge it in the state it is in?

Can I file a followup PR then? In particular I'm afraid that some of my calls to ISqrt might pass a negative number. And the launch angle & speed selection could be better for downhill firing...

Contributor

matija-hustic commented Oct 13, 2015

I wanted to clean up the code some more. And make sure no other problems appear... Are you sure you want to merge it in the state it is in?

Can I file a followup PR then? In particular I'm afraid that some of my calls to ISqrt might pass a negative number. And the launch angle & speed selection could be better for downhill firing...

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann
Contributor

obrakmann commented Oct 13, 2015

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Oct 13, 2015

Contributor

@matija-hustic: oh sorry, I wasn't aware of that. But sure, file a follow-up. It all seemed to work nicely for me, though.

Contributor

obrakmann commented Oct 13, 2015

@matija-hustic: oh sorry, I wasn't aware of that. But sure, file a follow-up. It all seemed to work nicely for me, though.

@matija-hustic

This comment has been minimized.

Show comment
Hide comment
@matija-hustic

matija-hustic Oct 13, 2015

Contributor

OK, I'll do that asap. And next time I'll ask for a "Has dependencies" label. I just hope the ISqrt calls don't cause problems.

Contributor

matija-hustic commented Oct 13, 2015

OK, I'll do that asap. And next time I'll ask for a "Has dependencies" label. I just hope the ISqrt calls don't cause problems.

}
// Attained height after ascent as predicted from upper part of incline surmounting manoeuvre
var predAttHght = loopRadius * (1024 - WAngle.FromFacing(vFacing).Cos()) / 1024 - diffClfMslHgt;

This comment has been minimized.

@pchote

pchote Oct 13, 2015

Member

If you do a followup pr, could you please put in some more readable names? We have plenty of spare vowels available for use in variables ;)

@pchote

pchote Oct 13, 2015

Member

If you do a followup pr, could you please put in some more readable names? We have plenty of spare vowels available for use in variables ;)

This comment has been minimized.

@matija-hustic

matija-hustic Oct 13, 2015

Contributor

Sure :) I would have already left the vowels before were I not afraid of the names being excessively long and ugly. E.g. predAttHght to predictedAttainedHeight basically doubles the length

@matija-hustic

matija-hustic Oct 13, 2015

Contributor

Sure :) I would have already left the vowels before were I not afraid of the names being excessively long and ugly. E.g. predAttHght to predictedAttainedHeight basically doubles the length

// within hHeightChange w-units all the while ending the ascent
// with vertical facing 0
for (var vFac = System.Math.Min(vFacing + info.VerticalRateOfTurn - 1, 63); vFac >= vFacing; vFac--)
if (!WillClimbWithinDistance(vFac, loopRadius, predClfDist, diffClfMslHgt)

This comment has been minimized.

@pchote

pchote Oct 13, 2015

Member

Also avoid hanging blocks - if you have an inner statement using braces, then please also put braces around the outer blocks.

@pchote

pchote Oct 13, 2015

Member

Also avoid hanging blocks - if you have an inner statement using braces, then please also put braces around the outer blocks.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Oct 13, 2015

Contributor

I'm just here to state that @matija-hustic is a hero™️, no more, no less.

Contributor

GraionDilach commented Oct 13, 2015

I'm just here to state that @matija-hustic is a hero™️, no more, no less.

readonly MissileInfo info;
readonly ProjectileArgs args;
readonly Animation anim;
// NOTE: Might be desirable to unhardcode the number -10
readonly WVec gravity = new WVec(0, 0, -10);

This comment has been minimized.

@reaperrr

reaperrr Oct 13, 2015

Contributor

If you do unhardcode this in a follow-up (I like the idea), personally I'd suggest leaving the minus here and only unhardcode the number, I think that would be more intuitive for modders (higher value = stronger gravity).

@reaperrr

reaperrr Oct 13, 2015

Contributor

If you do unhardcode this in a follow-up (I like the idea), personally I'd suggest leaving the minus here and only unhardcode the number, I think that would be more intuitive for modders (higher value = stronger gravity).

else
{
// Aim for the target
var vDist = new WVec(-relTarHgt, -relTarHorDist * (targetPassedBy ? -1 : 1), 0);

This comment has been minimized.

@Mailaender

Mailaender Oct 14, 2015

Member

Coverity detected dead code here (unreachable code, because of a logical contradiction) https://scan8.coverity.com/reports.htm#v24414/p10888/fileInstanceId=8951757&defectInstanceId=3159905&mergedDefectId=131343 Haven't investigated it further.

@Mailaender

Mailaender Oct 14, 2015

Member

Coverity detected dead code here (unreachable code, because of a logical contradiction) https://scan8.coverity.com/reports.htm#v24414/p10888/fileInstanceId=8951757&defectInstanceId=3159905&mergedDefectId=131343 Haven't investigated it further.

@hypercube33

This comment has been minimized.

Show comment
Hide comment
@hypercube33

hypercube33 Oct 16, 2015

Contributor

This seriously junked up the SAM sites for NOD / CnC Gold FYI. The SAMs now shoot missles into the ground short of their targets.

Contributor

hypercube33 commented Oct 16, 2015

This seriously junked up the SAM sites for NOD / CnC Gold FYI. The SAMs now shoot missles into the ground short of their targets.

@matija-hustic matija-hustic deleted the matija-hustic:missile branch Oct 21, 2015

matija-hustic added a commit to matija-hustic/OpenRA that referenced this pull request Oct 29, 2015

Next
Followup to #8717

* Gravity unharcoded
* Variable names more readable
* More smaller functions instead of few big ones
* Crash check reverted to strictly negative heights
* Retested SAMs, submarines, adv. guard towers

I'm happier with how the code looks right now. Feels cleaner to me at
least.

As far as I've seen, current functionality is OK. It remains to see if the
new features will cooperate once they're used in YAMLs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment