Page MenuHomeWildfire Games

Update range queries to account for entity size
Needs ReviewPublic

Authored by wraitii on May 22 2020, 1:03 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Range query checks currently do not take entity size into account. This can lead to returning too few entities and other unexpected behaviour.

This change simply adds size over-optimistically.

It likely leads to a slight performance decrease as this slightly increases all our ranges.


Discovered following Angen's comment here.

Test Plan

Agree that the change is desirable.

Event Timeline

wraitii created this revision.May 22 2020, 1:03 PM

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2199/display/redirect

Commenting to myself that I need to check what this does for gates.

Freagarach edited the summary of this revision. (Show Details)Jun 4 2020, 8:28 PM
Freagarach added inline comments.
source/simulation2/components/CCmpRangeManager.cpp
1218–1219

Why -1?

1252

Can this become negative?
(Also spaces.)

wraitii added inline comments.Jun 8 2020, 9:00 PM
source/simulation2/components/CCmpRangeManager.cpp
1218–1219

See comment below `// Subtract 1 since we round up from the real size and we prefer returning too many entities.`

Basically we want to be too permissive.

1252

it->second.size is rounded up so it would have to be compared against 0 (maybe possible?)

That being said, if it's negative it would also take a minRange of less than 1 to be relevant. Not sure it's actually a problem.

wraitii updated this revision to Diff 12251.Jun 11 2020, 4:46 PM

Lower gate opening range which was much too high now.

I've checked other range queries, and I don't expect this to have too much of an impact.
Buildings should detect entities coming in range earlier since they're relying on the attack range, but they were not adding the obstruction size so were quite pessimistic.
In other cases, I doubt the change is remarkable.

Owners added a subscriber: Restricted Owners Package.Jun 11 2020, 4:46 PM
Freagarach added a comment.EditedJun 11 2020, 4:49 PM

So this may "fix" the "archers standing just outside of the range of a building can destroy it" bug? (#3381)

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2397/display/redirect

So this may "fix" the "archers standing just outside of the range of a building can destroy it" bug?

cf https://trac.wildfiregames.com/ticket/3381

I think this should fix the range detection issue, however I believe the underlying issue has been fixed when the range checks were moved to ObstrucitonManager in whatever revision from last summer

bb added a subscriber: bb.EditedJun 11 2020, 8:12 PM

ccmpRangeManager.h claims targets and sources are points

I wonder whether this should become optional or not
Every call that depends on the fact the results are actually in range, might get a negative result

The implicit assumption in this new query is that all obstructions are circles, this should be stated somewhere. This does not fix the problem of buildings not attacking units while having the same range completely, since square buildings will have a longer diagonal.

source/simulation2/components/CCmpRangeManager.cpp
1255

Since here we actually check every entity and explicitly compute distances and stuff, we could be using the obstructionMan functions instead

1391

if we prefer having "too many" ents shouldn't we do - here?

In D2759#119726, @bb wrote:

Every call that depends on the fact the results are actually in range, might get a negative result

That's by design -> there's a comment somewhere saying this code needs to be fast, so it's not supposed to return exact results. Code that relies on this retuning exact results is broken-by-design.

square buildings will have a longer diagonal.

It should because the obstruction size is the diagonal I think, but I should check.

source/simulation2/components/CCmpRangeManager.cpp
1255

I think it'd be too slow, but we can give it a shot.

1391

Ah, you're probably right yes

wraitii updated this revision to Diff 12263.Jun 12 2020, 5:07 PM

Fix the minrange -= remark from bb

Out of curiosity I compared the range check with actually using the obstructionManager's isInPointRange.
Tested by running Combat Demo Huge for 200 turns while doing nothing.

Red is SVN
Green is the patch
Blue is using isInPointRange

So yeah, we're not using the proper functions here :p

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

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2408/display/redirect

bb added a comment.Jun 12 2020, 8:54 PM

Most likely it is due to some square-root done in the proper function and comparelength being smart

Will merge this in a few days.

bb added a comment.Fri, Jul 17, 1:48 PM
In D2759#119726, @bb wrote:

square buildings will have a longer diagonal.

It should because the obstruction size is the diagonal I think, but I should check.

You are right.

ccmpRangeManager.h claims targets and sources are points

don't forget about that.

I wonder whether this should become optional or not

One should notice that we already have ExecuteQueryAroundPos, so probably not required to have some optionality.

source/simulation2/components/CCmpRangeManager.cpp
1252

spaces for consistancy