Should not GuiTraceRay for quads beyond the floor until the far plane.

[saurtron]

Work done

• Pass minRayLength into GetQuadsOnRay instead of length.

Details

Length is the length til camera far plane, but minRayLength is actually the length until the ground/water (not sure why it's called minRayLength the name seems a bit misleading).

I believe this to be an error, resulting in querying the quadField for every quad until the end of the map, even when tracing relatively perpendicular. I was testing something else and wondering why TraceRay was tracing so far.

A typical ray trace around the screen will return around 10 quads with this, 50 or more without. It also depends where you are in the map since the more map in front of you more quads will be returned. With the fix the number of quads stays constant as you move around the map since it only depends on where the mouse cursor is, and where the ground is intersected.

This needs to be looked into closely for sure. On initial testing I didn't find a difference when applying the fix, but will have to look at all users of GuiTraceRay to see nothing funny is going on.

Also this means it wouldn't be respecting looking for units or features above the water, but probably that isn't a thing.

Before

see how the search area extends beyond the mouse pointer and even into the map side when there's no more map

After

[lhog]

Does your change work with underwater units?

[saurtron]

Does your change work with underwater units?

It should unless some caller is failing to set ignoreWater properly.

ignoreWater is the last parameter to GuiTraceRay though, and set to true as default, so pretty safe unless someone set it wrong explicitly.

I'll test, and review all callers (also on BAR lua side, if any) to make sure, because I think till now it wouldn't make a difference. Will report back when I have checked throughly.

[lhog]

Yeah you're right, I missed the circumstances how minRayLength is set.
Your change looks correct to me now.

[sprunk]

What about units behind a hill?

[saurtron]

What about units behind a hill?

I think if the're behind a hill you're not supposed to be selecting by clicking (I did a quick test and only things I could see through hills were numbers for metal spots).

[saurtron]

Your change looks correct to me now.

Yeah... I also thought maybe we need to overshoot a bit, to accout for unit size, in case the ray falls on one quad but the unit lives on a different one. But it looks like units already live in all quads they fit into to avoid problems with that.

Anyways this will need some testing to make sure everything is fine, I did skirmish a bit while testing with this on, and could not see a difference, but will keep testing in case I see anything.

[sprunk]

I think if the're behind a hill you're not supposed to be selecting by clicking (I did a quick test and only things I could see through hills were numbers for metal spots).

Visibility of the body itself doesn't necessarily matter. For one, selecting via dragging a box ignores terrain. But more importantly consider this, the unit I'm hovering over (with tooltip) is technically obscured behind a hill but it is actually very visible via things like low health smoke rising above the hill, shooting projectiles away from the hill, and being already selected which draws a circle through the hill:

The unit is clickable (in master, not this PR) but the other two aren't, because ground does block units that are both obscured if they are far enough away. There is currently 200 elmo lenience for this:

spring/rts/Game/TraceRay.cpp

Line 496 in b35d8d2

if ((minRayLength > 0.0f) && ((minRayLength + 200.0f) < minIngressDist)) {

I think I'd argue that I should be able to individually select the other two units that happen to be further away, perhaps even at arbitrary distance. And I agree that a game might rather only "really" visible units were selectable that way. So perhaps make it configurable? How about:

• turn the magic 200 constant a springsetting, i.e. add a macro like this
  

  spring/rts/Game/Game.cpp

  Lines 142 to 143 in 38a5ded

  	CONFIG(bool, ShowFPS).defaultValue(false).description("Displays current framerate.");
  	CONFIG(bool, ShowClock).defaultValue(true).headlessValue(false).description("Displays a clock on the top-right corner of the screen showing the elapsed time of the current game.");

• change this PR to

- quadField.GetQuadsOnRay(qfQuery, start, dir, minRayLength);
+ const auto lenienceBuffer = configHandler->GetFloat("selectThroughGroundLenienceBuffer"); // perhaps with a better name
+ const auto lenientRayLength = std::min(minRayLength + lenienceBuffer, length); // in case somebody sets lenience to 999999
+ quadField.GetQuadsOnRay(qfQuery, start, dir, lenientRayLength);

and apply the var in line 496.

[sprunk]

Another case to watch about would be if the unit is outside the map such that there isn't any ground for the trace to hit. IIRC it returns -1 in this case (which is why the minLength > 0 check in the line that currently checks for +200). The two common cases would be

1. units outside the map, probably easiest to reproduce with airplanes.
   
2. low angle into the sky. This one may be trickier because you also won't hit the y=0 plane for water surface.
   

[saurtron]

There is currently 200 elmo lenience

Right, I overlooked that part and should be taken into account adding lenience to minRayLength. As you say it can be made configurable too, will look into it.

1. units outside the map, probably easiest to reproduce with airplanes.

I wonder what quad those are assigned to. Will find out and see if something special needs to be done in that situation (likely yes).

2. low angle into the sky. This one may be trickier because you also won't hit the y=0 plane for water surface.

I don't see so much of a problem here, other than detecting no hit and thus not being able to apply minRayLengh. It's a bit of a corner case so personally don't think it's very important if in this case it still scans to far plane. Will check for that too.

[saurtron]

I applied a few changes that I believe account for all current concerns:

• Set maxRayLength to either a) ground intersection b) water intersection c) far plane.
• Add SelectThroughGround config variable to account for lenience and make it configurable, also adds it into maxRayLength, this way lenience will still work but be configurable like @sprunk requested.
• Use maxRayLength for max quadField ray distance.

This way we still get a limited ray as long as not pointing at the sky. The initial approach was still doing far plane ray when out of map, and I figured out that's also not the best. Let me know if you think intersecting the water plane there is a bad idea and some other level should be chosen instead.

I studied the case of airplanes out of map, and they get assigned to the closest quads on the side of the map, so this approach allows limiting the ray in all situations (except when pointing to the sky, where we kind of can't limit), while allowing proper interaction with out of map objects.

Btw, I noticed out of map objects break other things, like area attack XD (that I'm also working on at #1753), because it's not expecting the initial click is out of map at all!. I think that's better to leave for a further PR though.

[saurtron]

Just one concern of mine, regarding how we get the config variable, I don't think getting it with configHandler->GetFloat("nameofvariable") all the time is very optimal since it's basically reparsing it, looking for it in different sources...

I see other places usually load such variables inside constructors, but sadly here we don't have a constructor, so maybe GlobalConfig could be used instead and use it from there. What do you think? Personally I'd do that.

[sprunk]

Sounds good.

[saurtron]

Sounds good.

Done