Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Take radar error into account inside SelectCircleUnits. #1753

Merged
merged 2 commits into from
Nov 24, 2024

Conversation

saurtron
Copy link
Collaborator

@saurtron saurtron commented Nov 5, 2024

Work done

  • Take radar error into account inside SelectCircleUnits.
  • Improves circle selection for attack command.
  • Also implemented similar changes inside SelectRectangleUnits for consistency.

Details

  • Not saying it fixes circular selection since I believe perspective can still make it a bit awkward sometimes at the edges, but at least it won't feel totally random like now.
  • Also current situation can leak information since it does use real position.
  • Review carefully since I could be botching something related to synchronization, I hope not.
  • The method is only used by GiveCommandNet[CMD_ATTACK]->SelectAttackNet as far as I can see, so I didn't maintain old functionality. Might want to make a new method instead or have some kind of argument to control radar error usage.

Technical details

  • I'm adding TeamRadarErrorSize into the first quadField.GetUnitsExact query. I'm not sure whether this is the right amount here tbh. We need to add the radius so first query will give us all possible candidates.
  • After that, I take unit->GetErrorPos instead of unit->midPos to calculate distance, I think this is the right approach.
  • This will make the query more expensive, but at least results will be correct (I hope).

Before patch

Sorry for the terrible encoding quality need to figure this out.

circlesel_pre

After patch

circlesel_post

@sprunk
Copy link
Collaborator

sprunk commented Nov 5, 2024

Looks good. There's also a rectangle function alongside the circle, in theory that needs the same change applied as well but imo it's fine to just leave a fixme comment there and do it in a separate PR (I'm not sure how to even get the rectangle UI).

@saurtron
Copy link
Collaborator Author

saurtron commented Nov 5, 2024

Thing with maximum radar error, is depending if it's a square with TeamRadarErrorSize side or a circle with TeamRadarErrorSize radius, I need to account for it different. If it's a square then it will need to be multiplied by sqrt(2) to sum it into a radius.

Need to check that before this is accepted.

@saurtron
Copy link
Collaborator Author

saurtron commented Nov 9, 2024

There's also a rectangle function alongside the circle

Implemented SelectRectangleUnits too. Don't know how to test that but I think the implementation should be ok XD. Can always go back to the old version if you prefer. Anyways code review should be enough to be reasonably sure it will work imo the changes are not so great and very similar to the other method.

@saurtron
Copy link
Collaborator Author

saurtron commented Nov 9, 2024

Thing with maximum radar error, is depending if it's a square with TeamRadarErrorSize side or a circle with TeamRadarErrorSize radius, I need to account for it different. If it's a square then it will need to be multiplied by sqrt(2) to sum it into a radius.

Need to check that before this is accepted.

Regarding max error size, it looks like the max error is always in a circle relative to true position, so the value should be good as is in the PR (using allyTeamError directly to enlarge quadField search areas). It's total error is allyTeamError*unitError, where unitError is inside a 1 radius circle.

@lhog
Copy link
Collaborator

lhog commented Nov 24, 2024

LGTM

@lhog lhog merged commit 053b44e into beyond-all-reason:master Nov 24, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants