Implement a function to compare lengths within a percent error to avoid overkill. #6960

Open

real_tabasco_sauce wants to merge 1 commits from real_tabasco_sauce/0ad:RoughSort into main

pull from: real_tabasco_sauce/0ad:RoughSort

merge into: 0ad:main

0ad:main

0ad:diversify-linux

Conversation 15 Commits 1 Files Changed 8 +66 -27

real_tabasco_sauce commented 2024-08-24 01:08:25 +02:00

Contributor

Copy Link

This patch implements a new comparison function to allow sorting by distance within a percent error. Each unit gets a "RangeError" value which is read in UnitAI.js and passed on when range queries are set up. The Range Error is the percent error used in CompareLengthRough(). If RangeError is undefined or set to 0, the normal sorting procedure is undertaken.

Why:

Without player input, units attack the nearest unit with huge precision. This is fine for other games where smaller armies are involved, but with the large armies and battles in 0ad, it becomes problematic: If many units attack the same unit, much damage is wasted. For example, if more than 3 crossbows (2 for a26 vanilla) hit a ranged unit, each additional hit is wasted. For melee units, this high precision means that melee units will all try to attack the first unit until they try a new range query. A video of this problem is posted here: https://wildfiregames.com/forum/topic/118677-less-precise-sorting-in-range-manager/?do=findComment&comment=578105

With less precision when sorting enemies by distance:

• Battles appear more realistic/cool.

• Ranged units are less likely to overkill enemies. This should decrease the need to manually distribute damage across units when the units would be expected to attack multiple units.

• Melee units take different targets and path better since they don't first have to try and attack the absolute closest unit.

• The template value allows the precision to be tailored as needed on a per-unit basis, for instance, crossbowmen likely need a higher P value. For the time being, I have initialized every unit to have a RangeError of 2.

On performance:

• Two users reported no significant change in performance with this patch.

https://wildfiregames.com/forum/topic/118677-less-precise-sorting-in-range-manager/page/3/

• I reported a small performance increase in combat demo huge (blue is current patch, red is vanilla, green is a previous version of the patch) :

• @phosit reported a performance detriment on previous versions of the patch, but not on the most recent version which has greatly simplified math.

This patch implements a new comparison function to allow sorting by distance within a percent error. Each unit gets a "RangeError" value which is read in UnitAI.js and passed on when range queries are set up. The Range Error is the percent error used in CompareLengthRough(). If RangeError is undefined or set to 0, the normal sorting procedure is undertaken. Why: Without player input, units attack the nearest unit with huge precision. This is fine for other games where smaller armies are involved, but with the large armies and battles in 0ad, it becomes problematic: If many units attack the same unit, much damage is wasted. For example, if more than 3 crossbows (2 for a26 vanilla) hit a ranged unit, each additional hit is wasted. For melee units, this high precision means that melee units will all try to attack the first unit until they try a new range query. A video of this problem is posted here: https://wildfiregames.com/forum/topic/118677-less-precise-sorting-in-range-manager/?do=findComment&comment=578105 With less precision when sorting enemies by distance: - Battles appear more realistic/cool. - Ranged units are less likely to overkill enemies. This should decrease the need to manually distribute damage across units when the units would be expected to attack multiple units. - Melee units take different targets and path better since they don't first have to try and attack the absolute closest unit. - The template value allows the precision to be tailored as needed on a per-unit basis, for instance, crossbowmen likely need a higher P value. For the time being, I have initialized every unit to have a RangeError of 2. On performance: - Two users reported no significant change in performance with this patch. https://wildfiregames.com/forum/topic/118677-less-precise-sorting-in-range-manager/page/3/ - I reported a small performance increase in combat demo huge (blue is current patch, red is vanilla, green is a previous version of the patch) : <img width="933" alt="image" src="attachments/0e3bf4d6-d1c0-423c-a959-bcdc406f1540"> <img width="502" alt="image" src="attachments/464b120f-3fa1-439b-8104-1ba076adf4b0"> - @phosit reported a performance detriment on previous versions of the patch, but not on the most recent version which has greatly simplified math.

image.png

128 KiB

image.png

12 KiB

real_tabasco_sauce added 1 commit 2024-08-24 01:08:29 +02:00

Implement a function to compare lengths within a percent error to avoid overkill. d6f25d9c6a

real_tabasco_sauce requested review from Itms 2024-08-24 01:08:29 +02:00

real_tabasco_sauce requested review from wraitii 2024-08-24 01:08:29 +02:00

real_tabasco_sauce added 1 commit 2024-08-24 01:12:14 +02:00

don't use a higher precision value for LOS queries bccc57f3eb

Itms commented 2024-08-24 09:17:20 +02:00

Owner

Copy Link

Phabricator is still accessible for those who have an account, and it would be nice to keep track of what's been taken out of the review pile there.

So don't hesitate to

• write here that it's a continuation of D5282
• post in the diff that it's continued here
• close the diff

Thanks!

I'm putting phosit and Stan as reviewers since they had started looking at this diff, but it seems the review was a bit stalled. Don't hesitate to ask me to take a look and decide, preferably after next week when everything gitea-related is settled.

Phabricator is still accessible for those who have an account, and it would be nice to keep track of what's been taken out of the review pile there. So don't hesitate to - write here that it's a continuation of [D5282](https://code.wildfiregames.com/D5282) - post in the diff that it's continued here - close the diff Thanks! I'm putting phosit and Stan as reviewers since they had started looking at this diff, but it seems the review was a bit stalled. Don't hesitate to ask me to take a look and decide, preferably after next week when everything gitea-related is settled.

Itms removed review request for wraitii 2024-08-24 09:17:29 +02:00

Itms requested review from Stan 2024-08-24 09:17:47 +02:00

Itms requested review from phosit 2024-08-24 09:19:11 +02:00

real_tabasco_sauce commented 2024-08-24 20:44:58 +02:00

Author

Contributor

Copy Link

Don't hesitate to ask me to take a look and decide

Sure, feel free to take a look when you have time!

> Don't hesitate to ask me to take a look and decide Sure, feel free to take a look when you have time!

real_tabasco_sauce force-pushed RoughSort from c404c6d775 to f87d449d45 2024-08-24 23:28:16 +02:00 Compare

real_tabasco_sauce force-pushed RoughSort from 735a1386e6 to 1425e0bc44 2024-08-25 17:29:30 +02:00 Compare

real_tabasco_sauce commented 2024-08-27 19:56:43 +02:00

Author

Contributor

Copy Link

Any more thoughts here? @stan I'm not sure how well calling the entityDistanceOrdering in UnitAI.js would work, since I would need to avoid sorting by distance twice. So IMO, it makes sense to do the 'if (P)' in entityDistanceOrdering. Would it be much faster? I thought passing an additional parameter is in general pretty cheap.

I suppose I could also make an equivalent entityDistanceOrderingRough, which would remove the need to pass P to entityDistanceOrdering and move the if (P) check to the respective stable_sort calls within CCmpRangeManager.cpp. This would decrease the number of times we need to do if (P), but add some somewhat redundant code.

Also, I could post some more videos to the forum discussion to show the improvements to gameplay.

Any more thoughts here? @stan I'm not sure how well calling the entityDistanceOrdering in UnitAI.js would work, since I would need to avoid sorting by distance twice. So IMO, it makes sense to do the 'if (P)' in entityDistanceOrdering. Would it be much faster? I thought passing an additional parameter is in general pretty cheap. I suppose I could also make an equivalent entityDistanceOrderingRough, which would remove the need to pass P to entityDistanceOrdering and move the if (P) check to the respective stable_sort calls within CCmpRangeManager.cpp. This would decrease the number of times we need to do if (P), but add some somewhat redundant code. Also, I could post some more videos to the forum discussion to show the improvements to gameplay.

real_tabasco_sauce commented 2024-09-01 18:15:58 +02:00

Author

Contributor

Copy Link

Any more thoughts here? @stan I'm not sure how well calling the entityDistanceOrdering in UnitAI.js would work, since I would need to avoid sorting by distance twice. So IMO, it makes sense to do the 'if (P)' in entityDistanceOrdering. Would it be much faster? I thought passing an additional parameter is in general pretty cheap.

I suppose I could also make an equivalent entityDistanceOrderingRough, which would remove the need to pass P to entityDistanceOrdering and move the if (P) check to the respective stable_sort calls within CCmpRangeManager.cpp. This would decrease the number of times we need to do if (P), but add some somewhat redundant code.

Also, I could post some more videos to the forum discussion to show the improvements to gameplay.

@Itms thoughts on the patch? I did the above and it was a disaster for performance: https://wildfiregames.com/forum/topic/118677-less-precise-sorting-in-range-manager/?do=findComment&comment=581903

> Any more thoughts here? @stan I'm not sure how well calling the entityDistanceOrdering in UnitAI.js would work, since I would need to avoid sorting by distance twice. So IMO, it makes sense to do the 'if (P)' in entityDistanceOrdering. Would it be much faster? I thought passing an additional parameter is in general pretty cheap. > > I suppose I could also make an equivalent entityDistanceOrderingRough, which would remove the need to pass P to entityDistanceOrdering and move the if (P) check to the respective stable_sort calls within CCmpRangeManager.cpp. This would decrease the number of times we need to do if (P), but add some somewhat redundant code. > > Also, I could post some more videos to the forum discussion to show the improvements to gameplay. @Itms thoughts on the patch? I did the above and it was a disaster for performance: https://wildfiregames.com/forum/topic/118677-less-precise-sorting-in-range-manager/?do=findComment&comment=581903

real_tabasco_sauce force-pushed RoughSort from 1425e0bc44 to a0cca97d6f 2024-09-03 22:31:37 +02:00 Compare

real_tabasco_sauce added 1 commit 2024-09-04 00:18:44 +02:00

use +/- 5%, 10% 15% for P = 1,2,3 use base 100 for less discrete results. 5accd5aec1

real_tabasco_sauce commented 2024-09-04 00:27:48 +02:00

Author

Contributor

Copy Link

profile for the new version. We trade off a tiny bit of performance for more distributed target choices and a greater selection of % errors.

<img width="1614" alt="image" src="attachments/a50587d1-81df-4159-83fc-3cbfcedcf919"> profile for the new version. We trade off a tiny bit of performance for more distributed target choices and a greater selection of % errors.

image.png

300 KiB

real_tabasco_sauce added 1 commit 2024-09-04 00:45:38 +02:00

adjust P values for crossbowmen and bolt shooters. 293a038fdf

real_tabasco_sauce force-pushed RoughSort from 293a038fdf to a225cc1360 2024-09-04 01:00:49 +02:00 Compare

real_tabasco_sauce commented 2024-09-05 17:20:59 +02:00

Author

Contributor

Copy Link

@Itms could you take a look? I suppose if there are still concerns about performance, I could just enable it for crossbows. Although an inconsistency in unit behavior like that isn't ideal.

@Itms could you take a look? I suppose if there are still concerns about performance, I could just enable it for crossbows. Although an inconsistency in unit behavior like that isn't ideal.

real_tabasco_sauce commented 2024-09-06 17:48:20 +02:00

Author

Contributor

Copy Link

Maybe @freagarach, could you give your thoughts on this PR?

Maybe @freagarach, could you give your thoughts on this PR?

Itms commented 2024-09-07 11:00:51 +02:00

Owner

Copy Link

Hi @real_tabasco_sauce ! I'm unfortunately stomped by post-migration work on the CI, but I would like this to either move forward or be plainly refused, so that you can work on other things 🙂

• Your performance tests seem to give consistent results (similar perfs to vanilla, at worst a slight detriment)
• The videos you posted on the forums convinced me that this is a very nice feature for more realistic battles, and for the user experience both with auto-attack and ranged attacks
• The code structure looks clean and maintainable

So from this quick look, I am in favor of getting this merged. If @phosit is strongly against, I'll let him refuse the patch, as he reviewed it in previous iterations, but if he's undecided, I'm voting for an inclusion.

For a very superficial code review,

• I would like you to avoid using P as a variable name. It's fine in formulae in code comments, but bad for code readability. Use a spelled-out single word.
• It feels incorrect to name "precision" something that increases when the error increases. "variability" would work better, "dispersion" or "scatter" would work but only for ranged attacks I suppose
• Please revert the whitespace changes that are either incorrect (indentation) or unwanted (extra spacing around operators)
• Your comment about overflowing should be safeguarded with an assertion

I let someone else (if possible, @phosit or @Stan) go through the final review steps and help you squash and format your commit message.

Thanks for your work!

Hi @real_tabasco_sauce ! I'm unfortunately stomped by post-migration work on the CI, but I would like this to either move forward or be plainly refused, so that you can work on other things 🙂 - Your performance tests seem to give consistent results (similar perfs to vanilla, at worst a slight detriment) - The videos you posted on the forums convinced me that this is a very nice feature for more realistic battles, and for the user experience both with auto-attack and ranged attacks - The code structure looks clean and maintainable So from this quick look, I am in favor of getting this merged. If @phosit is strongly against, I'll let him refuse the patch, as he reviewed it in previous iterations, but if he's undecided, I'm voting for an inclusion. For a very superficial code review, - I would like you to avoid using `P` as a variable name. It's fine in formulae in code comments, but bad for code readability. Use a spelled-out single word. - It feels incorrect to name "precision" something that increases when the error increases. "variability" would work better, "dispersion" or "scatter" would work but only for ranged attacks I suppose - Please revert the whitespace changes that are either incorrect (indentation) or unwanted (extra spacing around operators) - Your comment about overflowing should be safeguarded with an assertion I let someone else (if possible, @phosit or @Stan) go through the final review steps and help you squash and format your commit message. Thanks for your work!

real_tabasco_sauce commented 2024-09-07 18:59:07 +02:00

Author

Contributor

Copy Link

Hi @real_tabasco_sauce ! I'm unfortunately stomped by post-migration work on the CI, but I would like this to either move forward or be plainly refused, so that you can work on other things 🙂

• Your performance tests seem to give consistent results (similar perfs to vanilla, at worst a slight detriment)
• The videos you posted on the forums convinced me that this is a very nice feature for more realistic battles, and for the user experience both with auto-attack and ranged attacks
• The code structure looks clean and maintainable

So from this quick look, I am in favor of getting this merged. If @phosit is strongly against, I'll let him refuse the patch, as he reviewed it in previous iterations, but if he's undecided, I'm voting for an inclusion.

For a very superficial code review,

• I would like you to avoid using P as a variable name. It's fine in formulae in code comments, but bad for code readability. Use a spelled-out single word.
• It feels incorrect to name "precision" something that increases when the error increases. "variability" would work better, "dispersion" or "scatter" would work but only for ranged attacks I suppose
• Please revert the whitespace changes that are either incorrect (indentation) or unwanted (extra spacing around operators)
• Your comment about overflowing should be safeguarded with an assertion

I let someone else (if possible, @phosit or @Stan) go through the final review steps and help you squash and format your commit message.

Thanks for your work!

Great! Thanks so much!
I would think range_error could be good, but I could go with variability too. For the assertion, should I just assert for the distances not being too large for performance purposes? In other words, is it necessary to actually check that the value would overflow?

> Hi @real_tabasco_sauce ! I'm unfortunately stomped by post-migration work on the CI, but I would like this to either move forward or be plainly refused, so that you can work on other things 🙂 > > - Your performance tests seem to give consistent results (similar perfs to vanilla, at worst a slight detriment) > - The videos you posted on the forums convinced me that this is a very nice feature for more realistic battles, and for the user experience both with auto-attack and ranged attacks > - The code structure looks clean and maintainable > > So from this quick look, I am in favor of getting this merged. If @phosit is strongly against, I'll let him refuse the patch, as he reviewed it in previous iterations, but if he's undecided, I'm voting for an inclusion. > > For a very superficial code review, > - I would like you to avoid using `P` as a variable name. It's fine in formulae in code comments, but bad for code readability. Use a spelled-out single word. > - It feels incorrect to name "precision" something that increases when the error increases. "variability" would work better, "dispersion" or "scatter" would work but only for ranged attacks I suppose > - Please revert the whitespace changes that are either incorrect (indentation) or unwanted (extra spacing around operators) > - Your comment about overflowing should be safeguarded with an assertion > > I let someone else (if possible, @phosit or @Stan) go through the final review steps and help you squash and format your commit message. > > Thanks for your work! Great! Thanks so much! I would think range_error could be good, but I could go with variability too. For the assertion, should I just assert for the distances not being too large for performance purposes? In other words, is it necessary to actually check that the value would overflow?

Itms commented 2024-09-07 19:51:03 +02:00

Owner

Copy Link

For the assertion, should I just assert for the distances not being too large for performance purposes? In other words, is it necessary to actually check that the value would overflow?

Use the Check...Overflow convenience functions used elsewhere in the file.

> For the assertion, should I just assert for the distances not being too large for performance purposes? In other words, is it necessary to actually check that the value would overflow? Use the `Check...Overflow` convenience functions used elsewhere in the file.

👍 1

Stan commented 2024-09-07 22:30:57 +02:00

Owner

Copy Link

Regarding the function parameter, use camelCase so rangeError instead of range_error.
Usually such a value is called epsilon i think but rangeError is nicer.

Regarding the function parameter, use camelCase so rangeError instead of range_error. Usually such a value is called epsilon i think but rangeError is nicer.

👍 1

real_tabasco_sauce force-pushed RoughSort from a225cc1360 to 493825fb93 2024-09-07 23:11:20 +02:00 Compare

real_tabasco_sauce force-pushed RoughSort from 493825fb93 to d99f659200 2024-09-07 23:11:55 +02:00 Compare

real_tabasco_sauce force-pushed RoughSort from d99f659200 to 4fea7fe143 2024-09-07 23:24:36 +02:00 Compare

real_tabasco_sauce force-pushed RoughSort from 4fea7fe143 to adec225455 2024-09-07 23:25:30 +02:00 Compare

real_tabasco_sauce commented 2024-09-07 23:26:21 +02:00

Author

Contributor

Copy Link

Ok, how does that look now?

Ok, how does that look now?

Stan commented 2024-09-07 23:35:18 +02:00

Owner

Copy Link

Looks alright :) You can also link the forum thread in the commit message if you wish to.

Looks alright :) You can also link the forum thread in the commit message if you wish to.

real_tabasco_sauce commented 2024-09-08 21:15:30 +02:00

Author

Contributor

Copy Link

Looks alright :) You can also link the forum thread in the commit message if you wish to.

I think its evolved enough that its better described in this PR anyway.

> Looks alright :) You can also link the forum thread in the commit message if you wish to. I think its evolved enough that its better described in this PR anyway.

real_tabasco_sauce force-pushed RoughSort from adec225455 to e90e1b119a 2024-09-08 21:15:43 +02:00 Compare

real_tabasco_sauce force-pushed RoughSort from e90e1b119a to ec1db2a104 2024-09-09 07:17:05 +02:00 Compare

real_tabasco_sauce force-pushed RoughSort from ec1db2a104 to fef84e145d 2024-09-11 19:00:37 +02:00 Compare

real_tabasco_sauce force-pushed RoughSort from fef84e145d to a4b7822715 2024-09-14 20:32:29 +02:00 Compare

real_tabasco_sauce force-pushed RoughSort from a4b7822715 to aebb699ac7 2024-09-15 21:49:22 +02:00 Compare

real_tabasco_sauce force-pushed RoughSort from aebb699ac7 to 8c2f94a6f6 2024-09-15 21:54:48 +02:00 Compare

real_tabasco_sauce commented 2024-09-15 21:55:46 +02:00

Author

Contributor

Copy Link

@Stan I added the forum link

@Stan I added the forum link

Some checks reported warnings

Hide all checks

checkrefs / checkrefs (pull_request) Successful in 1m3s

Required

Details

pre-commit / build (pull_request) Successful in 1m21s

Required

Details

0ad-windows/pipeline/pr-main This commit looks good

Details

0ad-freebsd/pipeline/pr-main This commit is unstable

Details

0ad-linux/pipeline/pr-main This commit is unstable

Required

Details

0ad-macos/pipeline/pr-main This commit is unstable

Details

Some required checks are missing.

You are not authorized to merge this pull request.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u RoughSort:real_tabasco_sauce-RoughSort

git checkout real_tabasco_sauce-RoughSort

Sign in to join this conversation.

Reviewers

No reviewers

Itms

Stan

phosit

Labels

Clear labels   

Closed

Duplicate

  

Closed

Fixed

  

Closed

Invalid

  

Closed

Needs info

  

Closed

Won't fix

  

Closed

Works for me

  

Difficulty

Hard

  

Difficulty

Medium

  

Difficulty

Simple

  

Needed for Beta

  

Needs Design Input

  

Needs Info

  

Pathfinding

  

Priority

1: Release Blocker

  

Priority

2: Must Have

  

Priority

3: Should Have

  

Priority

4: Nice To Have

  

Priority

5: If Time Permits

  

Regression

  

Theme

AI

  

Theme

Art & Animation

  

Theme

Atlas editor

  

Theme

Build & Packages

  

Theme

Core engine

  

Theme

Internationalization & Localization

  

Theme

Maps

  

Theme

Multiplayer Lobby

  

Theme

Music & Sound FX

  

Theme

Network

  

Theme

Non-game systems

  

Theme

Simulation

  

Theme

UI & Simulation

  

Theme

UI – Game setup

  

Theme

UI – In-game

  

Theme

UI – Miscellaneous

  

Theme

Website & Forum

  

Type

Defect

  

Type

Enhancement

  

Type

Task

No Label

Closed

Duplicate

Closed

Fixed

Closed

Invalid

Closed

Needs info

Closed

Won't fix

Closed

Works for me

Difficulty

Hard

Difficulty

Medium

Difficulty

Simple

Needed for Beta

Needs Design Input

Needs Info

Pathfinding

Priority

1: Release Blocker

Priority

2: Must Have

Priority

3: Should Have

Priority

4: Nice To Have

Priority

5: If Time Permits

Regression

Theme

AI

Theme

Art & Animation

Theme

Atlas editor

Theme

Build & Packages

Theme

Core engine

Theme

Internationalization & Localization

Theme

Maps

Theme

Multiplayer Lobby

Theme

Music & Sound FX

Theme

Network

Theme

Non-game systems

Theme

Simulation

Theme

UI & Simulation

Theme

UI – Game setup

Theme

UI – In-game

Theme

UI – Miscellaneous

Theme

Website & Forum

Type

Defect

Type

Enhancement

Type

Task

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

3 Participants

Notifications

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: 0ad/0ad#6960

No description provided.