[PATCH] Prettify unit/building selection ring

[Kieran P]

Prettify the unit/building selection ring. Change it from the 1px white line to a 1px white line, then a 2px fill of the users colour, then another 1px white line. Then apply this to all building, unit, animal, resource etc selection rings.

Attached is an example, please excuse the uglyness, it's an example, not a mockup ;-P

[historic_bruno]

Apparently we'll want to use textures and shaders instead, so the current implementation will have to do until then (doesn't seem like a minor change to me).

[vts]

Currently looking into this. Not as easy as it sounds, because ideally we would also like to be able to have all kinds of fancypants textures as selection overlays, and not just basic geometric forms like circles and rectangles.

The current idea is to use a single textured quad overlay. Buildings and units have footprints of all sorts and sizes though; to maintain a (roughly) 1px/2px/1px outline across all these sizes we'd need to create many textures of different sizes to prevent scaling due to texture stretching. Not quite sure yet if that's a good idea.

[vts]

I created a proof-of-concept implementation that uses a single quad for every selection outline. The other option is to make the overlay mesh consist of multiple quads for large buildings so it would stick to the terrain properly, but efficiency is a concern and this way allows for some simplifications that should also allow for less memory overhead and more efficient rendering. Philip mentioned that the terrain under buildings should be flattened anyway [citation needed :p], so it might be usable in the final product after all.

It does use a whole bunch of textures though, and I'm not quite sure yet how to manage these efficiently. I've created a small script to read out all the footprint sizes and generate the textures accordingly, and it turns out to be 103 unique footprints. This number will probably come down a bit if we allow "close-enough" approximations so we can reuse some of them.

Additionally, I made the selection outlines do a small alpha fade when they're activated per the todo in CCmpSelectable.

Anyway, here are some screenshots:
​http://i.imgur.com/n06vN.jpg
​http://i.imgur.com/Pr09u.jpg
​http://i.imgur.com/8e7t1.jpg

Hit me up on IRC or the forums if you've got any comments.

[vts]

See http://www.wildfiregames.com/forum/index.php?showtopic=15227 for a discussion about this ticket.

[vts]

So attached is my current WIP patch (in git format, I'm afraid). It does the following:

• Adds pretty unit overlays. Comes in two flavors:
  • Static outlines for structures; intended for entities that do not move (often). Reuses the territory boundary rendering code.
  • Dynamic outlines for units; batched for efficient rendering, intended for units that move around a lot. Uses textured quad overlays.
• Adds alpha fading to 50% on hover, 100% on select, per the TODO in CCmpSelectable
• Adds selection outlines for actors, following historic_bruno's atlas selection improvement patch

The dynamic overlays use textured quad overlays, which requires that each entity be assigned an overlay texture that's appropriate for its size (i.e. rectangular vs. circular, and of the proper size). I've left this as an exercise for the art department :o) There are a bunch of PNG binaries that come with this patch, one for every footprint size. Currently only circle/64x64.png is in use as the dynamic overlay texture for all units, so for large and/or rectangular units it will currently look bad (e.g. ships). I've added all of them so artists can easily pick and choose to adjust the textures for the various units as they see fit, and also because on Windows it's a pain to set up PyCairo to generate them in the first place.

Keep in mind, this is a WIP patch, and there are bits of code that still need to merged with existing code better or otherwise improved/removed. Off the top of my head:

• I have inadvertently reimplemented the SplitLine method in the sim2 Render helper (although in my defense, that method wasn't part of the public interface and the new version, SubdivideLines, is more generally usable).
• The XML element naming isn't quite perfect yet. "MainTexture" inside "Texture" sounds weird, but the less repetitive and more accurate "Texture" inside "Quad" is probably too technical.
• There are a ton of minor code rearrangements that have stacked up over time and might not necessarily be 'on topic' for this patch.
• The renderdata_id and cornerTexture* fields in !SOverlayDescriptor are remnants of old code and are not currently used, needs cleaning up (already fixed locally).

I'd like to have this patch reviewed, because I'm not sure I got everything quite right. In particular, some things I'd like to note/get reviewed:

• The QuadBatchKey and QuadBatchData structures are used by OverlayRenderer to group quads by texture for efficient rendering using a boost::unordered_map. I'd like to make sure I got the copy semantics, hashing and equality and everything right. The key uses shared_ptrs, so I think it should be ok, but I'd prefer to be sure.
• I've added a slightly modified version of the territory border overlay shader; I'm not familiar with writing shaders, and it seems to work fine, but nonethelesss I'd like to have it inspected if possible.
• I chose to add pointers to both a dynamic overlay and a static overlay to the CCmpSelectable component, and have only one of them be in use at any given time. Compared to alternatives like centralized registration in some sort of SelectionManager component, this has the advantage that they can be managed by their own component (CCmpSelectable can cleanly receive messages and invalidate them when needed). On the other hand, this does not lend itself well to further extensions of the possible overlay types (can't just keep adding pointers). For now though, I think this will do.

Will update this as I think of more stuff.

[Philip Taylor]

Initial thoughts:

Data files:

• shaders/overlay_selection.* looks almost identical to overlayline.*, so it'd probably be good to share the code between them. Looks like the only difference is selection uses vertex.color where line uses objectColor. See some later comment.
• The engine already prepends "art/textures/{ui,skins,skies,etc}" to pretty much all texture names read from data files, so it should probably do the same with selection textures, so the template XML files will just say <MainTexture>circle/64x64.png</MainTexture> etc.

graphics/Overlay.h

• Header files shouldn't include precompiled.h.
• Includes should be alphabeticalish.
• std::size_t should just be size_t.
• SOverlayQuad: Is it required/expected that the four corners be coplanar? If they aren't, what happens?

renderer/OverlayRenderer.cpp

• #include "boost/unordered_map.hpp" should be #include <boost/unordered_map.hpp>
• QuadBatchKey::QuadBatchKey: CTexturePtr arguments should typically be const CTexturePtr& instead, because that saves a shared_ptr copy (which requires atomic refcount operations).
• QuadBatchKey::hash: Shouldn't inherit from std::unary_function - you just need to implement the unary function concept (i.e. be callable as f(x)), you don't need to actually implement a concrete interface.
• QuadBatchKey::eq: Same.
• Since the definitions of hash and eq are so small, I think it'd be easier to read if they were defined inline in the class definition instead of at the end of the file. Also that may let the compiler inline the code better.
• eq is probably more cleanly handled by defining a QuadBatchKey::operator==, then you don't need to explicitly pass it to unordered_map<...>.
• hash is probably more cleanly handled by defining a global function "static size_t hash_value(const QuadBatchKey&)", then you don't need to explicitly pass it to unordered_map<...>.
• If you did "typedef std::pair<CTexturePtr, CTexturePtr> QuadBatchKey" then you wouldn't need to define your own struct and hash and eq at all, it should all work automatically. Maybe not good for clarity or extensibility, though.
• Better to use the newish VertexArray class instead of manually dealing with g_VBMan allocation. (See e.g. DecalRData.cpp or ParticleEmitter.cpp for usage examples.) (OverlayRenderer and PatchRData are the only code that's still using the old g_VBMan interface directly, and should be modernised for consistency. It'd be fine to leave that until later, though.)
• To minimise vertex buffer fragmentation, it'd probably be better to use a single VBO for the entire list of quads (plus a list of start+length per texture), instead of a VBO per texture.
• To minimise unnecessary reallocations, do vertices.reserve(batchRenderData.m_Quads.size()*4); (and for indices) before all the push_backing so that it starts the right size.
• There will probably be a very small set of colours in use at once (typically just one player colour, with one or two different alpha values since the selected units all fade in sync). Assuming that's true (might be nice to instrument the code to verify it), then instead of storing per-vertex colour data (which doubles the size of SVertex), it should be possible to put the colour into QuadBatchKey instead, and then load it as a uniform when rendering each batch. Call the uniform objectColor and then you can reuse the old overlayline shaders, I think.

renderer/OverlayRenderer.h

• If you're going to document the function parameters, you should probably document that the pointed-at object must remain valid until the end of the frame.

components/CCmpSelectable.cpp

• "TODO: we might be able to save some memory by de-duplicating these descriptors" - instead of de-duplicating entire descriptors, this might be a good opportunity to use the new CStrIntern, so the texture name strings take 4 bytes each (which should be sufficiently memory-efficient) instead of being however long the strings are. (Add some kind of CParamNode::ToCStrIntern() which converts to UTF-8 and constructs a CStrIntern, probably. Later we might want to change CParamNode to use CStrIntern internally, to save a bit more memory and to save conversion costs.)
• Instead of .ToFixed().ToFloat(), add a CParamNode::ToFloat (based on ToInt) to avoid needless conversions.
• The alpha fading needs to be framerate-independent, and it looks like it isn't. CMsgInterpolate tells the time since the last interpolate, so you should use that to fade by the appropriate amount.
• Should use fabsf, not fabs, when the argument is a float.
• UpdateStaticOverlay: Should check cmpPosition->IsInWorld() before doing GetPosition2D.
• UpdateStaticOverlay: Doesn't use cmpOwnership, cmpPlayerManager.
• g_Renderer.IsInitialised() needs to be CRenderer::IsInitialised(), else it'll die with a null pointer error if not initialised.

components/ICmpSelectable.h

• std::size_t again.
• SOverlayDescriptor::hash and operator== are unused.

Geometry.cpp

• Checking radius > 0 seems bogus. It won't prevent divide-by-zero, because e.g. SQR(1e-30) == 0. It won't prevent returning NaNs, because e.g. ChordToCentralAngle(4, 1) == acosf(1 - 16/2) == acosf(-7) == NaN.

[vts]

Very comprehensive review, Philip, many thanks :)

I've implemented/fixed all the issues you've mentioned, except for the one about moving the color data into the quad batch key to reduce the size of the vertices. Even though the fading is tied to MT_Interpolate messages in this new version, I'm not convinced that the overlays will typically fade in lockstep (e.g. when bandboxing), and performance would suffer if they don't because each distinct alpha value would then introduce an extra draw call.

[vts]

Updated version of the patch after incorporating Philip's comments

[vts]

For reference, I've attached the updated patch. Should be ready to be committed.

[vts]

In 11623:

Pretty unit selection overlay rings. Fixes #824.

[Kieran P]

The new selection rings has a bug which triggers this error, but it only happens randomly and I haven't been able to catch it again to do a backtrace:

VertexBuffer.cpp(181): Assertion failed: "m_Handle"
Assertion failed: "m_Handle"
Location: VertexBuffer.cpp:181 (UpdateChunkVertices)

It doesn't appear to be only me though. It's happened for Michael (Mythos) as well.

[vts]

In 11654:

Allocate VBO buffers in OverlayRenderer only after graphics capabilities and shader path have been properly determined. Fixes #824.

[vts]

In 11944:

Added elapsed real time (as opposed to elapsed simulation time) to MT_Interpolate messages. Fixes leftover TODO from #824. Refs #824.