HomeWildfire Games

Fix bonus multiplier issue in rP22754 (multiple attack effects) and consider…

Description

Fix bonus multiplier issue in rP22754 (multiple attack effects) and consider more entities in EntitiesNearPoint

  • the bonus multiplier would self-multiply with every effect, so an attack with multiple effect would have a broken multiplier.
  • EntitiesNearPoint used IID_Resistance, which is now facultative. To not miss entities, it now returns all entities owned by enemy players. For GAIA it checks for Health to avoid returning trees.

This means enemy units have higher priority over missed projectiles, but that should generally be OK.

This also adds simple tests.

Reported By: Freagarach

Differential Revision: https://code.wildfiregames.com/D2322

Event Timeline

Freagarach added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/helpers/Attacking.js
291

Whoops, still fails when stray missiles with capture attack try to hit a health-less GAIA catafalque. Sorry for not noticing this earlier...

elexis added a subscriber: elexis.Sep 29 2019, 6:49 PM

It currently still has Health, but indeed it should have that stripped some day.

The other thing as noted today on IRC was that this function now does 2 rangemanager calls when one over all entities should suffice.

Also it sounds a bit fragile to modify the input argument players, it means you have to check that all current callers pass copies that are not used afterwards and then hope that the future callers also dont use an object they assumed would only be an input argument.

I don't know if its true, but wraitii said the function (today on IRC) is also used for capturing. If that is so, then I assume you want to check for the attack (status effect?) type which component interface ID should be passed to the rangemanager (IID_Health, IID_Capturable, ...).

Ideally one would do a range query over entities which only have the right IID indeed (although that also can lead to strange stuff).
I would vote for one range query, thus taking into account trees as well, however that allows one to "hide" in trees, which is cool but ought to be introduced differently I guess. But that is a design decision ;)

Ideally one would do a range query over entities which only have the right IID indeed (although that also can lead to strange stuff).

What strange stuff?

I would vote for one range query, thus taking into account trees as well, however that allows one to "hide" in trees, which is cool but ought to be introduced differently I guess. But that is a design decision ;)

Whut

Why not just merge the two queries and always filter for Health if its an attack and always filter for Capturing if its a capture attack? Trees shouldnt be attacked

What strange stuff?

A projectile could hit an entity farther away from the observed point of impact if the entity which is closer does not have the right IID.

Why not just merge the two queries and always filter for Health if its an attack and always filter for Capturing if its a capture attack? Trees shouldnt be attacked

One would need to pass the IID to this function and do more than one query. I just think it would be difficult. E.g. imagine an arrow which only has a status effect which influences the movement speed of an entity, one would need to check what IIDs can be affected by the attack and pass that to this function.

Also it shouldn't be IID_Health but IID_DamageReceiver which is implemented by IID_Armour, it seems like that changed to IID_Resistance (futile!).

A projectile could hit an entity farther away from the observed point of impact if the entity which is closer does not have the right IID.

It sounds more like a bug than hiding behind a tree, there is a camouflaging ticket and unfinished patch btw.

One would need to pass the IID to this function and do more than one query.

Passing the IID sounds right.
Why do more than one query? If you want to get attackable entities, search for attackable entities, regardless of playerID?

I just think it would be difficult. E.g. imagine an arrow which only has a status effect which influences the movement speed of an entity, one would need to check what IIDs can be affected by the attack and pass that to this function.

Which is one of the things one should think about before implementing, let alone committing that.
But I suppose you want the StatusEffectReceiver IID for that.

Why do more than one query? If you want to get attackable entities, search for attackable entities, regardless of playerID?

This is not about attackable entities, but about entities which can be hit by a stray projectile. But for every IID "in" the projectile one would need a query (or one combined one). I'm not talking about players anymore ;)

But I suppose you want the StatusEffectReceiver IID for that.

Oh yeah, ofcourse ^^

Why do more than one query?

For every IID "in" the projectile one would need a query (or one combined one).

I suppose the int requiredInterface in CCmpRangeManager.cpp could become a std::vector<int> if one wants to query multiple IIDs at the same time.
(But should question whether its really needed)

Freagarach added inline comments.Oct 4 2019, 8:06 AM
/ps/trunk/binaries/data/mods/public/simulation/helpers/Attacking.js
293

forum post
If only GAIA is to be considered, this range check won't have any players.

elexis raised a concern with this commit.Oct 4 2019, 12:17 PM
  1. error messages see https://code.wildfiregames.com/rP23012#38687

WARNING: CCmpRangeManager: No owners in query for entity 0
when you choose a female farmer to build a house with a male soldier in the game, this warning will appear
If only GAIA is to be considered, this range check won't have any players.

  1. Doing two range queries sounds instead of one sounds like much more performance cost. Perhaps someone shoudl measure it with Engine.GetMicroseconds() or Profiler2 to see it in actual numbers (perhaps with a nonvisual replay before and after the patch).
  1. Chosing IID_Health is wrong for capturing. It sounds like one should query for the IID that the subsequent code will process on (so that might have been DamageReceiver, StatusEffectReceiver or WhateverReceiver) (see comments above)
This commit now has outstanding concerns.Oct 4 2019, 12:17 PM
Angen added a subscriber: Angen.Jan 30 2020, 6:55 PM
Angen added inline comments.
/ps/trunk/binaries/data/mods/public/simulation/helpers/Attacking.js
291

there are 2 ways how to approach this.
1st, get from query any entity and damage dealing function will deal with the applying or not applying damage based on receiver existence (cons - too many entities returned - trees, mines, etc).
2nd, add create queries based on receivers (cons - one query per possible receiver e,g, health, capture, etc)

293

in case player owns some entity without health (tree as resource for some reason), it will be returned here but looks like will not be the case.

wraitii requested verification of this commit.May 20 2020, 7:08 PM
This commit now requires verification by auditors.May 20 2020, 7:08 PM
elexis removed an auditor: elexis.May 20 2020, 7:48 PM
This commit no longer requires audit.May 20 2020, 7:48 PM