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

add radius threshold to box selections #1607

Closed

Conversation

loveridge
Copy link
Collaborator

Adds a unitdef for a spherical threshold for box selecting units. A default value of 0 leaves the current behavior unchanged.

#671

Copy link
Collaborator

@sprunk sprunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks decent, though not following existing volumes as far as allowed shapes is pretty sad.

rts/Game/SelectedUnitsHandler.cpp Outdated Show resolved Hide resolved
rts/Game/SelectedUnitsHandler.cpp Outdated Show resolved Hide resolved
@@ -383,6 +383,7 @@ class CSolidObject: public CWorldObject {
LocalModel localModel;
CollisionVolume collisionVolume;
CollisionVolume selectionVolume;
float selectionRadius = 0.0f;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there some neat convenience function to check CollisionVolume's distance to a plane? It would be good not to converge on a worse solution (for example only being able to do a sphere would fail hard for elongated units).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately only a distance to point exists

rts/System/SpringMath.cpp Outdated Show resolved Hide resolved
@lhog
Copy link
Collaborator

lhog commented Jul 19, 2024

It's tempting to propose you use selectionVolume CollisionVolume::GetBoundingRadius()/GetBoundingRadiusSq() for that or some quick projection of the selection volume onto the viewport left-right/top-bottom plane.

@sprunk
Copy link
Collaborator

sprunk commented Jul 19, 2024

Using the existing selection volume (without an opt-out) would be pretty bad, see the middle section (with sc2 protoss screenshots) of #671 (comment)

@lhog
Copy link
Collaborator

lhog commented Jul 19, 2024

Using the existing selection volume (without an opt-out) would be pretty bad, see the middle section (with sc2 protoss screenshots) of #671 (comment)

Perhaps you're right.

@@ -307,6 +307,8 @@ void CUnit::PreInit(const UnitLoadParams& params)

harvestStorage = unitDef->harvestStorage;

selectionRadius = unitDef->selectionRadius;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you copy this as a separate variable, so it could be changed on a per-unit basis?
Otherwise u->unitDef->selectionRadius might work equally well in places you now use u->selectionRadius.
Not a big deal, just curious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I remember seeing some gadget that would scale unit sizes, maybe from sprung? Although I do not know how well supported that is with other engine things like collision volumes.

@lhog
Copy link
Collaborator

lhog commented Jul 19, 2024

LGTM overall 👍

rts/Sim/Units/UnitDef.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@sprunk sprunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@loveridge loveridge marked this pull request as draft September 2, 2024 18:42
@loveridge
Copy link
Collaborator Author

This needs a proper box/sphere intersection test in order to not get false positives in the drag selection.

@loveridge loveridge closed this Nov 2, 2024
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