Page MenuHomeWildfire Games

Projectile hit animation
ClosedPublic

Authored by Mate-86 on Oct 1 2017, 6:26 PM.

Details

Summary

Explosion/dust animation is played when the projectile hits the ground.
(deleting the implementation design because it has been changed multiple time since then.)

Test Plan

Test in the game that explosion animation is properly played at missile hit.

Some unit tests for the ProjectileManager would be nice.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 3832
Build 6665: Vulcan BuildJenkins

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
leper added inline comments.Nov 21 2017, 9:18 PM
source/simulation2/components/CCmpProjectileManager.cpp
44

There are still some unused parameters here. If a TODO might need them in the future we can add them once we need them.

Mate-86 marked 3 inline comments as done.Nov 22 2017, 6:59 PM
In D945#41634, @leper wrote:

for (type varname : iterable) creates a copy of the current element which is placed in varname. Use for (type& varname : iterable) to just keep a reference, and if you want to make it obvious that the loop does not modify the elements in iterable use for (const type& varname : iterable).

Ok, I've fixed this.

source/simulation2/components/CCmpProjectileManager.cpp
44

Such as?

Mate-86 updated this revision to Diff 4325.Nov 22 2017, 7:00 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
../../../source/simulation2/components/CCmpProjectile.cpp:31:43: warning: unused parameter ‘componentManager’ [-Wunused-parameter]
  static void ClassInit(CComponentManager& componentManager)
                                           ^
../../../source/simulation2/components/CCmpProjectile.cpp:63:38: warning: unused parameter ‘serialize’ [-Wunused-parameter]
  virtual void Serialize(ISerializer& serialize)
                                      ^
../../../source/simulation2/components/CCmpProjectile.cpp:67:45: warning: unused parameter ‘paramNode’ [-Wunused-parameter]
  virtual void Deserialize(const CParamNode& paramNode, IDeserializer& deserialize)
                                             ^
../../../source/simulation2/components/CCmpProjectile.cpp:67:71: warning: unused parameter ‘deserialize’ [-Wunused-parameter]
  virtual void Deserialize(const CParamNode& paramNode, IDeserializer& deserialize)
                                                                       ^
Build (debug)...
../../../source/simulation2/components/CCmpProjectile.cpp:31:43: warning: unused parameter ‘componentManager’ [-Wunused-parameter]
  static void ClassInit(CComponentManager& componentManager)
                                           ^
../../../source/simulation2/components/CCmpProjectile.cpp:63:38: warning: unused parameter ‘serialize’ [-Wunused-parameter]
  virtual void Serialize(ISerializer& serialize)
                                      ^
../../../source/simulation2/components/CCmpProjectile.cpp:67:45: warning: unused parameter ‘paramNode’ [-Wunused-parameter]
  virtual void Deserialize(const CParamNode& paramNode, IDeserializer& deserialize)
                                             ^
../../../source/simulation2/components/CCmpProjectile.cpp:67:71: warning: unused parameter ‘deserialize’ [-Wunused-parameter]
  virtual void Deserialize(const CParamNode& paramNode, IDeserializer& deserialize)
                                                                       ^
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
leper added inline comments.Nov 22 2017, 9:57 PM
source/simulation2/components/CCmpProjectileManager.cpp
44

Ah, it seems I just can't read.

Is there a reason for the newline (and then using a tab, making this strange to read. Also the { belongs on the next line (see Coding_Conventions).

wraitii requested changes to this revision.Nov 25 2017, 2:46 PM
wraitii added a reviewer: bb.
wraitii added a subscriber: bb.

Okay so I'm going to change my opinion for the second time, sorry about that. The way to go is actually to pass three additional parameters to LaunchProjectile, one for the projectile actor, one for the "hit" actor, which may be made optional I guess and one for the lifetime of the "hit".

The definition of this projectile should actually go in the attack.js component (pinging @bb on this) so that different attacks could have different projectiles.

The changes to source/simulation2/components/CCmpProjectileManager.cpp require a few changes, see inline.

I'd be OK with commandeering this revision if you feel tired of working on this.

This revision now requires changes to proceed.Nov 25 2017, 2:46 PM

See also leper's comment.

source/simulation2/components/CCmpProjectileManager.cpp
61

Just remove the commented code entirely then.

116

No need to virtual this one I believe.

132

feel like "Lifetime" would describe this better. Still not a fan of "hit" though.

188

Generally preferable to call this cmpSourceVisualActor (the interface is poorly named).

194

{ on newline.

199

same

289

Switch these two ifs to early "continue" instead.

292

I'd rather define those above their first use.

382–385

Feel like you could put these on one line now.

bb added a comment.Nov 25 2017, 3:01 PM

Didn't read the whole discussion, but when adding some data about projectiles in the attack component, it might be good to make that an own optional entry. So we get a cmpAttack.Ranged.Projectile containing all relevant data to that. this could perhaps also include gravity, spread and projectileSpeed (imo that are all entries which requie a projectile, splash f.e. could also happen without a projectile eventually). For the secondary attacks I already planned such a thing, but doing it here, is fine too (would save me changing all templates :P)

Mate-86 marked 16 inline comments as done.Nov 25 2017, 7:09 PM
Mate-86 added inline comments.
source/simulation2/components/CCmpProjectileManager.cpp
44

The cited coding convention says "Try to avoid very wide lines". I'm happy to use a better indentation if you can suggest one.

132

How about impactAnimationLifeTime or finalAnimationLifeTime? Or you can check the thesaurus :) http://www.thesaurus.com/browse/hit?s=t

Also what @bb said.

binaries/data/mods/public/simulation/templates/template_unit_mechanical_siege_onager.xml
3

(Given some of the other comments above this is going to change anyway, but try to keep the "top" (not quite, but well) level tags sorted, makes finding things a lot easier.)

source/simulation2/components/CCmpProjectileManager.cpp
44

Yes, however that line is already quite long at the point where you split it. So either split it earlier, possibly align (that is not tabs, someone will come along and point out that this doesn't look nice with a tabwidth or 3 or 5) the parameters after a line break.

But the main point was that the { is not in the right place.

Mate-86 marked 10 inline comments as done.Nov 26 2017, 12:10 PM
Mate-86 updated this revision to Diff 4385.Nov 26 2017, 12:24 PM

Projectile definition is moved into the Attack/Ranged part of the entity template. Open questions:

  1. Does that make sense to cover this changes with unit tests (JS and C++ part)?
  2. @wraitii proposed to do the projectile actor defaulting (fall back to visual actor, see CCmpProjectileManager.cpp L191) in JS instead of in C++. I thought this further and how about replacing the source parameter of LaunchProjectileAtPoint with sourcePosition and in this case there is no need for the source entity in CCmpProjectileManager at all? For this the function cmpSourceVisualActor->GetProjectileLaunchPoint(); need to be exposed to JS.

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Stan added a comment.Nov 26 2017, 12:35 PM
  1. I think if you have time/motivation, more tests never hurt. If you don't feel like doing them here in this diff, you could create another revision and add a dependency on this one.
wraitii requested changes to this revision.Nov 26 2017, 12:46 PM

@wraitii proposed to do the projectile actor defaulting (fall back to visual actor, see CCmpProjectileManager.cpp L191) in JS instead of in C++. I thought this further and how about replacing the source parameter of LaunchProjectileAtPoint with sourcePosition and in this case there is no need for the source entity in CCmpProjectileManager at all? For this the function cmpSourceVisualActor->GetProjectileLaunchPoint(); need to be exposed to JS.

I think it's much better through JS, because then the C++ code doesn't even need to be aware of visual actor, which is a good thing as it decouples components (particularly C++ ones). You could even remove the header include.

I think you should replace "hit" with "Impact" indeed, as that's clearer (to me anyway).

Apart from that this seems quite reasonable to me and a good change going forward, so ?

binaries/data/mods/public/simulation/components/Attack.js
152

I don't think this one should be optional, or alternatively you should make "either or" with the impact actor.

533

This this would be better one one line imo, screens are wide and there's not a very logical way to split it.

source/simulation2/components/CCmpProjectileManager.cpp
109

Same here.

This revision now requires changes to proceed.Nov 26 2017, 12:46 PM
Imarok removed a subscriber: Imarok.Nov 26 2017, 3:14 PM
Mate-86 updated this revision to Diff 4395.Nov 26 2017, 3:59 PM
Mate-86 edited the summary of this revision. (Show Details)

Thinking about removing LaunchProjectile method because it's only used by the LaunchProjectileAtPosition.
Not sure whether it makes sense to introduce a new interface method in ICmpProjectileManager.cpp for LaunchProjectileAtPosition but with less parameters. If the actorName is not defined then the LaunchProjectile returns earlier without creating the projectile (not a valid scenario).
Unit tests will come most probably in a separate patch.

wraitii added inline comments.Nov 26 2017, 5:36 PM
binaries/data/mods/public/simulation/components/Attack.js
152

Think this lets you have several actorName, I'm not sure whether that works or not... But the proper definition is probably a fair bit annoying.

source/simulation2/components/CCmpProjectileManager.cpp
289

Don't delete this if, add a new parameter to projectile so that the impact actor creation only happens once.

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Mate-86 updated this revision to Diff 4411.Nov 27 2017, 6:40 PM
Mate-86 marked 2 inline comments as done.
Mate-86 added inline comments.
binaries/data/mods/public/simulation/components/Attack.js
152

I could add maxExclusive="2" which does not help against having 2 actorNames but at least there cannot be more.

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
wraitii requested changes to this revision.EditedNov 30 2017, 1:51 PM

One more pass and I think this is committable, I believe it's quite a good improvement both behaviour-wise and code-wise (which as I've learned over time indeed often goes together).

Thanks for the work on this so far!

binaries/data/mods/public/simulation/components/Attack.js
533

@elexis I think we don't use this but some kind of custom deep copy?

534

I think it's great that this magic number is no longer in the C++, but now you could even make it part of the template definition of the projectile.

537

This could use a comment saying we fallback to the old method or something.

source/simulation2/components/CCmpProjectileManager.cpp
129

Reorder this struct to minimise the size it takes. Bools should be put together at the end I think. Use god bolt.org to check on different compilers if you want.

179

This might be personal but I like to keep unrelated early exit separate.
@leper opinion?

source/simulation2/components/CCmpVisualActor.cpp
390

What's the reason for this change? Just that we don't expose CVector3D to js?

This revision now requires changes to proceed.Nov 30 2017, 1:51 PM
Mate-86 updated this revision to Diff 4541.Dec 4 2017, 9:15 PM
Mate-86 marked an inline comment as done.
Mate-86 added inline comments.
binaries/data/mods/public/simulation/components/Attack.js
534

Do you mean adding this offset to the unit templates which means I have to add the projectile property to the templates to all existing ranged units?
Is only the y coordinate needed or all the 3 dimensions?

source/simulation2/components/CCmpProjectileManager.cpp
129

I checked this godbolt and the order of the variables does not affect the size of the generated assembly code (if that was the question).

source/simulation2/components/CCmpVisualActor.cpp
390

Yep. This function is exposed to JS code and the DEFINE_INTERFACE_METHOD_XXX does not support CVector3D as parameter.

wraitii requested changes to this revision.Dec 10 2017, 2:59 PM

Looking good, here are some final few remarks.

binaries/data/mods/public/simulation/components/Attack.js
527

{ on the next line

534

Add it as optional inside your "Projectile" bit.

We should then move the gravity and projectile speed bits of Attack.js inside of this, but we'll do that in a separate patch.

536

{ on the next line

source/simulation2/components/CCmpProjectileManager.cpp
127

Move this after timeHit;

131

Make this a std::unique_ptr<std::wstring>. It'll take far less memory in the case where it doesn't exist and won't be much slower when it does exist.
I'd suggest std::optional but that's C++17.
Also add a

static_assert(sizeof(Projectile) == 80, "Projectiles is 80 bytes large")

bit, we like to be explicit about our sizes.

You're right about the order not mattering here anyhow though.

source/simulation2/components/CCmpVisualActor.cpp
390

Ok. Maybe add a comment to make it explicit that we don't actually care about it being fixed then, as that tends to mean "this is network synched".

This revision now requires changes to proceed.Dec 10 2017, 2:59 PM
Mate-86 marked 3 inline comments as done.Dec 17 2017, 7:39 PM
Mate-86 added inline comments.
source/simulation2/components/CCmpProjectileManager.cpp
131

I did some experiment with this unique_ptr member but it looks like I have to define move constructor and operator= for Projectile class in order to use push_back and swap on std::vector<Projectile> m_Projectiles. Otherwise I get errors like:
1>C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\xmemory0(577): error C2280: 'std::unique_ptr<int,std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : attempting to reference a deleted function

Defining move constructor and operator= seems pretty complicated to me because all the struct members need to be listed there. Is this what we really want?

131

static_assert(sizeof(Projectile) == 80, "Projectiles is 80 bytes large")

I tried to get the size of the struct which is not trivial because I had to copy all the Projectile and Vector members into a console application and print out the sizeof value. The end result is 92 but I get the compilation error: CCmpProjectileManager.cpp(149): error C2338: Projectiles is 92 bytes large

static_assert(sizeof(Projectile) == 92, "Projectiles is 92 bytes large");

wraitii accepted this revision.Dec 18 2017, 12:10 PM

Needs fixing the formatting issue before committing, otherwise OK given inline comments.

If nobody (on the team) minds, I'd like to commit this myself as I want to upload a diff depending on it at around the same time.

source/simulation2/components/CCmpProjectileManager.cpp
131

Hello @Mate-86, re your second question:

That's normal, that's not how you do it :p . Because of "alignment", a somewhat complicated thing which will be better explained elsewhere, you can't simply add up all the sizes to get the size of the struct. In fact, this one I believe takes up 96 bytes, which means it is 16-bytes aligned (96/16 = 6).

The std::unique_ptr version only takes up 80 bytes (80/16 = 5).

You're right about the move and copy constructor, that is more advanced C++ indeed. I think I'll upload a separate patch after I've committed this then. I'll address the format issues before committing.

This revision is now accepted and ready to land.Dec 18 2017, 12:10 PM
Stan added a comment.Dec 18 2017, 6:52 PM

This feature is something I look forward to.
If the formatting is not too hard maybe you could fix it yourself given how hard mate has working on this. And since you did the review I guess you can commit it.

Mate-86 updated this revision to Diff 4814.Dec 18 2017, 10:48 PM
Mate-86 marked 4 inline comments as done.

Tried to address all the code formatting issues.

In the meantime I wanted to upload all my changes since it turned out the C++ comments I cannot address right now. Let's have a look if you like it or not and feel free to switch back to my Dec 4 patch if it's better.

binaries/data/mods/public/simulation/components/Attack.js
534

There is a half ready change about LaunchProjectile attribute where I defined the coordinates as elements but probably would be better as attributes.

instead of

<LaunchPoint>
	<X>0</X>
	<Y>3</Y>
	<Z>0</Z>
</LaunchPoint>

use
<LaunchPoint x="0" y="3" z="0"/>
or use coordinate type if there is one exist.

This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Dec 23 2017, 10:27 AM