If a projectile miss its main target there is a query to check if the projectile doesn't actually hit another entity.
To not hardcoded a number, currently the radius of the query is twice the distance between the projectile position and the main target position.
That radius as no real meaning, we just know that it will not be too big. The wrong thing is that if the main target is dead or has not anymore position (garrisoned for example), we will just fall in an early return and the actually hit target won't be damaged.
We could use an hardcoded number, 20 would handle all the case (even if we hit the corner of the case) but could be also too big (so we will check too much collision).
We could use also another not hardcoded distance.
Details
Details
- Reviewers
- None
- Trac Tickets
- #4595
-
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 2126 Build 3452: Vulcan Build (Windows) Jenkins Build 3451: Vulcan Build Jenkins Build 3450: arc lint + arc unit
Event Timeline
Comment Actions
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
| 259| 259| Damage.prototype.TargetKilled = function(attacker, target, attackerOwner)
| 260| 260| {
| 261| 261| let cmpAttackerOwnership = Engine.QueryInterface(attacker, IID_Ownership);
| 262| |- let atkOwner = cmpAttackerOwnership && cmpAttackerOwnership.GetOwner() != -1 ? cmpAttackerOwnership.GetOwner() : attackerOwner;
| | 262|+ let atkOwner = cmpAttackerOwnership && cmpAttackerOwnership.GetOwner() != -1 ? cmpAttackerOwnership.GetOwner() : attackerOwner;
| 263| 263|
| 264| 264| // Add to killer statistics.
| 265| 265| let cmpKillerPlayerStatisticsTracker = QueryPlayerIDInterface(atkOwner, IID_StatisticsTracker);
Executing section XML GUI...
Executing section Python...
Executing section Perl...http://jw:8080/job/phabricator_lint/112/ for more details.
| binaries/data/mods/public/simulation/components/Damage.js | ||
|---|---|---|
| 128 | 20 seems like an unfortunate hardcoding. If the world unit is changed or if mods add huge templates, this might fail. Maybe it already fails for things like the wonder where a catapult hits before this change but now not anymore? Perhaps the spread of the attacker (or twice that) should be used? (Sounds like work though) | |
| 129 | ||
Comment Actions
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/1454/ for more details.
| binaries/data/mods/public/simulation/components/Damage.js | ||
|---|---|---|
| 128 | Sure (see summary). If such a number was used, it would need at least a comment. I don't see obvious link with the spread. | |
| binaries/data/mods/public/simulation/components/Damage.js | ||
|---|---|---|
| 128 | Expecting the center vs footprint boundary thing, we will basically just check the first one of the array (and having a big range query is really bad if nothing else is hurt imo). The current thing was a rough approximation. We could add method (in range query or here) or something like that to get the first encoutered entity, but then we would need a threshold, wich is in fact exactly what we have here. | |
| binaries/data/mods/public/simulation/components/Damage.js | ||
|---|---|---|
| 128 | I have an idea (but there is not enough room in this margin). | |
Comment Actions
Anectdote:
in source/simulation/Collision.h?rev=4437
// maximum radius at which we check for entities (if some entity is much bigger than this, that's bad) #define COLLISION_RANGE 20