Pick object to follow when activity window is shown

[falbrechtskirchinger]

Using the "Follow object" function is quite cumbersome, as one has to place the object near the center of the Observation Window first, which can be difficult at higher game speeds.

I propose picking the object to follow at the time the mouse click to open the Activity Window occurs. Simultaneously, I didn't want to completely replace the existing behavior and implemented an expiration mechanic. After some amount of time (TBD) the picked object is expired and the Observation Window reverts to the existing behavior. For the duration the picked object is valid, the tooltip shows which object will be followed when activated (currently – as a placeholder only – using typeid(obj).name()).

I can think of less clunky, more elaborate solutions, but it's honestly not an important enough feature, that I'd care to expend the effort. The goal is just to go from (IMO) "unusable" to "usable".

To-do:

• Remove debug code. (After Implement GUI scaling for HighDPI displays and accessibility (SDL2 only) #1594.)
• Make node objects return a proper name for the Follow %s format string and replace std::type_info placeholders.
• Experiment with deprioritizing some figures (at least carriers) during picking. (Multiplying the distance by some factor might be enough.)
• The selection radius is quite high. Should be narrower and probably proportional to the zoom factor to keep it to a constant DPI-adjusted pixel radius.
• Get feedback: Is the expiration mechanism needed? I.e., should the current follow mechanic still be accessible?
• Add a setting to control the behavior.
• Consider unifying IsValid() and HasExpired() or otherwise make the use of each function clearer.
• MoveToFollowedObj() can probably be rewritten using TrackPickedMovableObject().

[Flow86]

Split out GetPointsInRadius() changes and consider using Boost.Coroutine2.

please refrain of using any multithreading if not 1000% necessary

[falbrechtskirchinger]

Split out GetPointsInRadius() changes and consider using Boost.Coroutine2.

please refrain of using any multithreading if not 1000% necessary

Coroutines have nothing to do with multithreading, i.e., they are used for sequential processing and – unlike threads – are in no way concurrent. I suggest using them as a fallback for std::generator, since that's a C++23 feature that isn't widely available yet. GetPointsInRadius() et al. return a vector and eagerly compute all the elements, even when they aren't needed (because the loop iterating over them aborts early, for example).

I really don't like my current solution of the Foreach*() function variants where a boolean is returned to abort or continue execution. The control flow is unnatural and harder to follow. Also, constraining myself to C++14 (which I inferred to be the used standard) made things very verbose (see for example the detail::InvokeCallback() trickery in lieu of if constexpr).

I'll more than likely submit a PR and you can decide if what I come up with is acceptable to you.

[falbrechtskirchinger]

At this early stage, I'd prefer feedback on the basic mechanic. Especially, the idea that the picked object expires after a few seconds, reverting to the current behavior of picking the object near the center of the view.
Does that make sense? How did the original game work?

[Flamefire]

At this early stage, I'd prefer feedback on the basic mechanic.

Can you describe in the initial post what exactly you want to achieve with this? I.e. how you imagine this to work exactly. That is easier to discuss than to infer/guess that from the source/changeset which is rather large.

How did the original game work?

IIRC it only had the option to follow the object currently in the cross/center of the window once you click the button.
But @Spikeone might know better.

BTW: An alternative to your ForeachPointInRadius might be using CheckPointsInRadius which looks like it can do what you intend already. I'd like to avoid having another similarly named function.

[Spikeone]

Tested it a bit and thats the behavior:

• When opening the window, it initially follows the closest unit (a settler or animal)
• It doesn't matter if it's a hostile, neutral or friendly unit
• If the targeted unit is destroyed (e.g. an animal dies, a soldier dies, a settler enters a building and stays) the observation window changes it's mode to "free", staying at the current position
• If the camera icon is clicked in targeted mode, it switches to free mode staying at the current position
• If the camera icon is clicked in free mode, it switches to targeted mode, targeting the closest unit
• In free mode, there is a crosshair visible (see screenshot)
• In targeted mode, there is no crosshair visible
• If a carrier is targeted, who enters a building, and leaves it again (e.g. carries something into the HQ - except those between the HQ and HQ-Flag), the target persists
• If a unit is targeted, which enters a building (e.g. a forester), the target is lost
• It is possible to follow invisible targets (not visible by FoW), although I'd count this as a bug
• It is not important, that the current target is visible on screen
• Units are searched around the currently targeted node (by the observation window)
• there is no priority (e.g. settlers > animals)
• right clicking into the observation window immediatly stops targeted mode
• It is possible to target the same unit multiple times
• Enlarging the observation window ignores disabling FoW (ingame settings, not via cheat), which clearly is a bug
• It is possible to target non roaming units (e.g. a miner who carries out some ore)
• There is no range limit as far as I have tested
• When targeting a unit, minimizing the observation window and the unit is no longer a valid target, the window seems to target a random unit, which feels like a bug. Doing the same while leaving the window visible, it switches to free mode staying at the last position
• Building names / Percentages are not visible in the window
• Right-Click closing works on every area (including buttons) except the rendered area
• There is a limit of 2 observation windows

Targeted mode:

Free mode:

Hope this helps

[falbrechtskirchinger]

Wow, thanks. There're quite a few things you discovered that don't work as intended.
As I stated, this is very much work-in-progress. I'm working on something else at the moment and want to avoid context-switching. I'll get back to this soon.

In any case, this has to be rebased on top of #1594, as that has implications for object picking due to how the zoom calculation is affected. I'll keep the debug code around until then.

[falbrechtskirchinger]

Can you describe in the initial post what exactly you want to achieve with this?

Right. I meant to expand the initial post later. I've done so now.

BTW: An alternative to your ForeachPointInRadius might be using CheckPointsInRadius which looks like it can do what you intend already. I'd like to avoid having another similarly named function.

I completely missed that function. Makes perfect sense to get rid of the changes to MapBase and also side-steps the std::generator issue completely. Thanks! 👍

[Flamefire]

Thanks for the update, that makes it clear and sounds very good!

Make node objects return a proper name for the Follow %s format string and replace std::type_info placeholders.

I don't think that can be easily done and neither think is actually that useful for the required work. Might be enough to have "Follow centered object"/"Follow picked object" instead.

The selection radius is quite high. Should be narrower and probably proportional to the zoom factor to keep it to a constant DPI-adjusted pixel radius.

I'm fine with the kinda higher radius

Get feedback: Is the expiration mechanism needed? I.e., should the current follow mechanic still be accessible?

I think it's a good idea. After unfollow the current mechanic should be used, same as after expiration. Might even have a separate button but not sure if there is enough space to make this look good.

Consider unifying IsValid() and HasExpired() or otherwise make the use of each function clearer.

Absolutely: Get rid of the latter by folding it into the former. Or do you require the separation at some point?

Some implementation notes:

• You cannot store the object instance/pointer but only the ID as it may be destroyed at any point, or disappear into a building
• I'd rather prefer to have the Pick* functions return the PickableObject instead of using reference parameters which are hard to reason about
• Similar: The other functions might be members of PickableObject instead of iwObservate making them cleaner by getting rid of the reference parameter. Might call the "track" function simply track or update or updatePosition or so to be clear what it does
• Maybe a ctor of PickableObject would be good which auto-calculates the expiration time right there instead of expireIn

[falbrechtskirchinger]

Make node objects return a proper name for the Follow %s format string and replace std::type_info placeholders.

I don't think that can be easily done and neither think is actually that useful for the required work. Might be enough to have "Follow centered object"/"Follow picked object" instead.

I've used your suggestion for now, but do still find that rather useful. Postponed to a separate PR and if I discover along the way, that it's not worth the effort, I'll drop it. (I don't think it's too involved TBH.)
Minor change: "Follow object near the center", as that more accurately describes what's happening.

The selection radius is quite high. Should be narrower and probably proportional to the zoom factor to keep it to a constant DPI-adjusted pixel radius.

I'm fine with the kinda higher radius

It doesn't make a lot of sense to me to pick an object ridiculously far away from the cursor when the objective is to pick what the user clicked on.

Get feedback: Is the expiration mechanism needed? I.e., should the current follow mechanic still be accessible?

I think it's a good idea. After unfollow the current mechanic should be used, same as after expiration. Might even have a separate button but not sure if there is enough space to make this look good.

There's room for a button, but it involves more work, including creating a new asset (or can we reuse something?). Maybe in the future?

Consider unifying IsValid() and HasExpired() or otherwise make the use of each function clearer.

Absolutely: Get rid of the latter by folding it into the former. Or do you require the separation at some point?

Well, I did and it bit me in the end.

1. I need to detect when the object expires to update the interface/tooltip.
2. When I start following an object, I need to cancel its expiration.

Consequently, we now have another bool in iwObservate to detect the transition from valid to invalid (needed in iwObservate::Draw_()) and I added CancelExpiration().
It does simplify the if conditions/control flow in some places and is overall easier to understand IMO.

Some implementation notes:

* You cannot store the object instance/pointer but only the ID as it may be destroyed at any point, or disappear into a building

Thanks for the pointer. ;-) Fixed!

* I'd rather prefer to have the `Pick*` functions return the `PickableObject` instead of using reference parameters which are hard to reason about

Done.

* Similar: The other functions might be members of `PickableObject` instead of `iwObservate` making them cleaner by getting rid of the reference parameter. Might call the "track" function simply `track` or `update` or `updatePosition` or so to be clear what it does

Also done. Went further and moved it into a separate file. I felt it had grown large enough.

* Maybe a ctor of `PickableObject` would be good which auto-calculates the expiration time right there instead of `expireIn`

This is now done in the static member functions via a bool expire parameter.

---

Notes:

• I'm using move-semantics to pass the PickedMovableObject from iwAction to iwObservate which invalidates the object stored in iwAction. Not technically needed since iwAction doesn't stick around long enough to cause problems, but is more correct anyway.
• I suspect there are still some literal edge cases (top left corner of the map is on screen), where things might break.
• It would probably be cleaner to move some of those calculations (to wrap and scale coordinates) into GameWorldView instead of querying GetZoomFactor() and doing the calculations in PickedMovableObject.
• I'm not sure if the edge case ever worked correctly.
• There's a race condition if the object expires during a frame and IsValid() returns different results for the same draw call. It doesn't really matter, though.

[Flamefire]

I'm using move-semantics to pass the PickedMovableObject from iwAction to iwObservate which invalidates the object stored in iwAction. Not technically needed since iwAction doesn't stick around long enough to cause problems, but is more correct anyway.

I don't think this is required it just makes things a bit more complicated as one now has to know what a moved-from pickable object is. So I'd just leave it. BTW: You can use std::exchange for the move ctor and even for the current call if you need:

std::move(pickedObject_) -> std::exchange(pickedObject_, PickedMovableObject{})

[falbrechtskirchinger]

For some figures, there's a significant position error due to an offset that's applied for drawing. I've spent some time exploring different solutions:

1. Add a virtual function noMovabe::getDrawInfo() that collects all textures and computes all offsets, then perform the actual drawing in a separate DrawInfo::draw() call. The offset information would be accessible via noMovable::getOffsetToCenter() which returns a reasonable approximation of the offset to the center of mass of the figure. This is quite invasive and a lot of work.
2. Compute the offset to center in the draw call itself and cache it in the figure. This is equally invasive and less clean.
3. Only compute the absolutely necessary offset in the draw call of some select figures (most notably the builder) and calculate the offset from a figure's feet (which is the DrawPoint we're currently working with) to its center of mass once based on a representative texture. Minimally invasive and maybe not the cleanest solution.

Option 3 seems the most reasonable and I'd leave that work for a follow-up PR. This error isn't new, so it's not a regression and this PR doesn't make things worse. One exception might be trying to pick a builder which can move outside the pick radius (in pixels, not map points) while working on a building site.

With these future changes in mind, it might make sense to polish the debug code (DebugOverlay + DrawCross(), DrawPlus(), DrawPolygon() (for drawing a circle, actually)) and formally include it. I have some ideas to make it usable in other places, should it ever be needed and I'd define some macros based on NDEBUG so it won't be active in release builds.
Otherwise, I'll just hang on to it locally for the follow-up PR.

This PR may also contribute a few more [tweakables]:

• pickObjectAtCursor (enables the features in this PR; default on?)
• pickObjectRadius
• deprioritizationFactor (exact naming TBD) or more granular (but this is overkill IMO):
  • animalDeprioritizationFactor
  • carrierDeprioritizationFactor
  • ...

Lastly, another issue identified is that noFighting will need special handling at some point (same follow-up PR, probably).