Page MenuHomeWildfire Games

Fix a bug when attacking mirage with a range attack
ClosedPublic

Authored by fatherbushido on Jun 5 2017, 2:25 PM.

Details

Reviewers
elexis
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP19791: Fix a Damage bug. Don't try to damage the main target when it doesn't have…
Trac Tickets
#4276
Summary

Currently when a unit attack a mirage, the projectiles are fired.
But as the entity is a mirage and has not a DamageReceiver the damage are not done.
I recall the missile hit current logic:

  • If the main target is hit, do damage
  • Else do a query (filtered by DamageReceiver) and try to find an hit entity and damage hit.

So for the moment, I suggest to just add a check for the DamageReceiver in the first step.

(I have other things in mind)

Test Plan

You can check with the attached replay as observer than the red ptol cc will get damage with the patch and not without.
(target: Ent: 150, Mirage: 171)

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

fatherbushido created this revision.Jun 5 2017, 2:25 PM

binaries/data/mods/public/simulation/components/Damage.js
119 ↗(On Diff #2441)

Comment could be improved as often :)

Vulcan added a subscriber: Vulcan.Jun 5 2017, 7:10 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1486/ for more details.

Vulcan added a comment.Jun 5 2017, 7:14 PM
Executing section Default...
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'cmpAttackerOwnership'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Damage.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/simulation/components/Damage.js
| 265| 265| Damage.prototype.TargetKilled = function(attacker, target, attackerOwner)
| 266| 266| {
| 267| 267| 	let cmpAttackerOwnership = Engine.QueryInterface(attacker, IID_Ownership);
| 268|    |-	let atkOwner =  cmpAttackerOwnership && cmpAttackerOwnership.GetOwner() != -1 ? cmpAttackerOwnership.GetOwner() : attackerOwner;
|    | 268|+	let atkOwner = cmpAttackerOwnership && cmpAttackerOwnership.GetOwner() != -1 ? cmpAttackerOwnership.GetOwner() : attackerOwner;
| 269| 269| 
| 270| 270| 	// Add to killer statistics.
| 271| 271| 	let cmpKillerPlayerStatisticsTracker = QueryPlayerIDInterface(atkOwner, IID_StatisticsTracker);

binaries/data/mods/public/simulation/components/Damage.js
| 120| »   let·cmpDamageReceiver·=·Engine.QueryInterface(data.target,·IID_DamageReceiver)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/136/ for more details.

Thanks linter

Vulcan added a comment.Jun 6 2017, 8:08 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1496/ for more details.

Vulcan added a comment.Jun 6 2017, 8:09 AM
Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/143/ for more details.

elexis added a subscriber: elexis.Jun 6 2017, 10:16 AM

Currently it decreases the health of the mirage but not the original entity?
The check is also useful once we can remove the DamageReceiver from relics.
Should we catch the mirage case and test for collision with the according non-miraged entity?
Comment seems fine

In D605#25059, @elexis wrote:

Currently it decreases the health of the mirage but not the original entity?

No, because there is no DamageReceiver for the mirage.

The check is also useful once we can remove the DamageReceiver from relics.

Yes, it fix a more general issue than the Mirage thing.

Should we catch the mirage case and test for collision with the according non-miraged entity?

I first think of things like that (getting the parent or doing thing in Mirage component) but the non-miraged entity will be caught just after in the range query.

In fact that first part (check the main target could be skiped and we could only rely on the range query - wich does the DamageReceiver filtering - but it has at two inconveniences, and is out of the scope of that diff).

Comment seems fine

ok :-)

elexis accepted this revision.Jun 13 2017, 4:52 PM

Nice replay, thanks for the patch!

This revision is now accepted and ready to land.Jun 13 2017, 4:52 PM
This revision was automatically updated to reflect the committed changes.