[PATCH] Always show hero selection rings

[historic_bruno]

To make heroes stand out more with the new selection rings, they should have a variation of their selection ring always shown (faded color or 50% alpha?). Because we might want this behavior for more than heroes, it seems better to make this a general property of the ​Selectable component rather than special casing heroes.

[historic_bruno]

example from Mythos_Ruler

[Deiz]

I've implemented this with an XML element for the hero templates. A unit with the <AlwaysVisible /> element as a child of <Overlay> will have a permanent selection overlay.

I've done two versions of the patch. The first simply sets the minimum alpha for <AlwaysVisible /> units' selection overlays to 40% alpha. Note that the GUI's mouseover overlay uses 50% alpha. The difference is subtle, but I couldn't go much lower without the overlay disappearing into the scenery.

The second version of the patch keeps the overlay at a minimum of 50% alpha (same as mouseover), but reduces saturation by one-third. I find the higher alpha keeps the overlay more visible, and the desaturation is sufficient to make mouseover on heroes more obvious. The implementation is uglier than the first patch, as there doesn't seem to be a way to get the parent entity in order to check its selection status; I instead rely on alpha 0 == unselected.

Here's a gallery of the second implementation: ​https://imgur.com/a/x6fgj

[Deiz]

Patch updated to remove spaces from XML elements (per Philip) and passed a boolean to confirm selection status, as suggested by historicbruno.

[Deiz]

Original patch is missing the all-important js call-site tweaks.

[historic_bruno]

Finally getting around to reviewing this :)

I like the desaturation concept more and it has the bonus of being closer to Michael's example:

A few comments/bugs:

• There's a problem initializing the AlwaysVisible state. It's noticeable when saving and loading a game with heroes, opening a map in Atlas, or playing and resetting a simulation test in Atlas. For some reason the heroes all revert back to having no selection ring. I've not had time to look into why this is.
• For greyscale player colors, there's no visual difference between unselected and mouseover rings. While that's a given with the desaturation effect, I think it also shows a flaw in the design. The hero selection rings are too subtle due to the 50% alpha, and the saturation isn't enough to really distinguish mouseover.

The point is to make them stand out :) So I wonder if we couldn't bump the minimum alpha and make the mouseover value somewhere in between.

• We could also try a combination of desaturation and alpha for the mouseover state, so something like this:
  • Unselected, always visible: 66% saturation, 60% alpha
  • Mouseover: 85% saturation, 75% alpha
  • Selected: 100% saturation, 100% alpha

I have no idea how that would work, it might be awful. Either way I think the mouseover state in your current patch is a good target for the unselected hero rings:

[Deiz]

I like the idea of bumping up the alpha. The last version of the patch uses 50% alpha because that's already pretty low-contrast on a lot of terrain. I only left the mouseover alpha as-is because I didn't know if 50% was deliberately chosen.

Regarding the initialization issues: It seems that the SetSelectionHighlight() is ever only called by the engine when an OwnershipChanged message is received. In all other instances, it's up to the GUI to call it.

I did some debugging and noticed some odd behavior. When starting a new match, the selectables are initialized, and then the ownership changes, leading to the colour being set with the correct alpha.

I put in a little function to detect when m_AlphaMin is set, and put a gdb breakpoint inside it. Here you can see it triggering (as expected) for a hero: ​http://pastebin.com/m8EJTJ7f

Yet, bizarrely, it also triggers for particle entities: ​http://pastebin.com/tLNvDHqA

I haven't taken an in-depth look at how exactly loading differs from starting a new match, but I did notice that it will enter the 'if (m_AlphaMin > 0.)' conditional exactly twice as often. My hypothesis is that normal initialization is being done, followed by the usual OwnershipChanged message, and then everything is reinitialized, clobbering the m_Color values and never calling SetSelectionHighlight() again. I believe the Atlas issue is similar.

Edit: CCmpSelectable::Deserialize() re-runs Init(), which is responsible for the second run during loading. If Init must be run there, but the OwnershipChanged messages are received much earlier than that, it seems there's a load order problem.

[Deiz]

Okay, I've got a version that seems fairly bulletproof.

Talking to Philip` I tried a few approaches, such as trying to determine player colour inside of CCmpSelectable::Deserialize. Unfortunately, that doesn't work, as scripted components are loaded quite late. Another option was to insert code into each of the disparate loading code paths ("Load game" from the menu, quick-load, Atlas reset, etc.) but that seemed... very ugly.

In the latest patch I've opted to cache the player colour when first rendering a selection overlay. At this point, all the necessary components are all but guaranteed to exist, and if not, the caching will retry each frame until it succeeds once. I've tested it in all the above loading scenarios, and it seems to work well.

[ben]

In 12148:

Implements always visible hero selection rings, based on patch by Deiz/F00. Fixes #1365

[historic_bruno]

I went with the approach of bumping minimum alpha values: up to 65% for unselected heroes, and 75% for all hovered entities. Desaturation was left at 33%. This looked good in my testing. If not we can always change it :)

[Doménique]

Sorry to reopen this ticket, but I noticed a small bug.

It seems there is a problem when a hero unit is garrisoned in any building or ship. The selection stars remain at the spot where the hero 'left the world' (it does not matter whether the unit was selected at that time or not). As soon as a hero is ungarrisoned the stars are shown at the correction, current location of the hero. So it appears that hiding the selection ring (as it happens for normal units) does not work any longer, because the AlwaysVisible parameter overrules it. See also the screenshot I added.

I assume the solution is in this file (​http://trac.wildfiregames.com/changeset/12148/ps/trunk/source/simulation2/components/CCmpSelectable.cpp), although when updating the overlay it already appears to check whether the entity is in the world. I have made a patch that resolves the problem at render time, but I feel this is inefficient. Someone with a better grasp of the codebase can probably solve this with the PositionChanged message handling.

Here is a ​link to an ZIP-file with a test map with heroes and some entities that allow to garrison behaviour.

[Doménique]

Hero selection rings remain at their last in-world location when garrisoning

[Doménique]

Resolves the non-disappearance of the hero selection ring at RenderSubmit

[Deiz]

In 12239:

Detect when units aren't in the world and hide their selection rings. Based on patch by dvangennip. Refs #1365.

[Deiz]

In 12241:

Slightly speed up actor rendering; have selectables properly handle fog of war, etc. Refs #1365.