#824 closed enhancement (fixed)
[PATCH] Prettify unit/building selection ring
Reported by: | Kieran P | Owned by: | vts |
---|---|---|---|
Priority: | Must Have | Milestone: | Alpha 10 |
Component: | Core engine | Keywords: | patch |
Cc: | Patch: |
Description
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
Attachments (4)
Change History (27)
by , 13 years ago
Attachment: | unit_selection_ring.jpg added |
---|
comment:1 by , 13 years ago
Milestone: | Alpha 6 → Alpha 7 |
---|
comment:2 by , 13 years ago
Owner: | set to |
---|
comment:3 by , 13 years ago
Component: | Game engine → Game Art |
---|
comment:4 by , 13 years ago
Owner: | changed from | to
---|
comment:5 by , 13 years ago
Owner: | removed |
---|
comment:6 by , 13 years ago
Milestone: | Alpha 7 → Alpha 8 |
---|
comment:7 by , 13 years ago
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.
comment:8 by , 13 years ago
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.
comment:9 by , 13 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
See http://www.wildfiregames.com/forum/index.php?showtopic=15227 for a discussion about this ticket.
comment:10 by , 12 years ago
Milestone: | Alpha 8 → Alpha 9 |
---|
comment:11 by , 12 years ago
Priority: | Nice to Have → Should Have |
---|
by , 12 years ago
Attachment: | pretty_unit_rings_WIP_01mar12.patch added |
---|
by , 12 years ago
Attachment: | pretty_unit_rings_WIP_01mar12_textures.zip added |
---|
comment:12 by , 12 years ago
Component: | Game Art → Game engine |
---|
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.
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.
comment:13 by , 12 years ago
Keywords: | review added |
---|---|
Summary: | Prettify unit/building selection ring → [PATCH] Prettify unit/building selection ring |
comment:14 by , 12 years ago
Milestone: | Alpha 9 → Alpha 10 |
---|
comment:15 by , 12 years ago
Keywords: | patch added |
---|---|
Priority: | Should Have → Must Have |
comment:16 by , 12 years ago
Initial thoughts:
Data files:
shaders/overlay_selection.*
looks almost identical tooverlayline.*
, so it'd probably be good to share the code between them. Looks like the only difference isselection
usesvertex.color
whereline
usesobjectColor
. 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 besize_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 beconst CTexturePtr&
instead, because that saves ashared_ptr
copy (which requires atomic refcount operations).QuadBatchKey::hash
: Shouldn't inherit fromstd::unary_function
- you just need to implement the unary function concept (i.e. be callable asf(x)
), you don't need to actually implement a concrete interface.QuadBatchKey::eq
: Same.- Since the definitions of
hash
andeq
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 aQuadBatchKey::operator==
, then you don't need to explicitly pass it tounordered_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 tounordered_map<...>
.- If you did "
typedef std::pair<CTexturePtr, CTexturePtr> QuadBatchKey
" then you wouldn't need to define your ownstruct
andhash
andeq
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 withg_VBMan
allocation. (See e.g.DecalRData.cpp
orParticleEmitter.cpp
for usage examples.) (OverlayRenderer
andPatchRData
are the only code that's still using the oldg_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 forindices
) before all thepush_back
ing 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 intoQuadBatchKey
instead, and then load it as a uniform when rendering each batch. Call the uniformobjectColor
and then you can reuse the oldoverlayline
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 newCStrIntern
, 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 ofCParamNode::ToCStrIntern()
which converts to UTF-8 and constructs aCStrIntern
, probably. Later we might want to changeCParamNode
to useCStrIntern
internally, to save a bit more memory and to save conversion costs.) - Instead of
.ToFixed().ToFloat()
, add aCParamNode::ToFloat
(based onToInt
) 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
, notfabs
, when the argument is a float. UpdateStaticOverlay
: Should checkcmpPosition->IsInWorld()
before doingGetPosition2D
.UpdateStaticOverlay
: Doesn't usecmpOwnership
,cmpPlayerManager
.g_Renderer.IsInitialised()
needs to beCRenderer::IsInitialised()
, else it'll die with a null pointer error if not initialised.
components/ICmpSelectable.h
std::size_t
again.SOverlayDescriptor::hash
andoperator==
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
.
comment:17 by , 12 years ago
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.
by , 12 years ago
Attachment: | 824_selection_rings_21apr12.patch added |
---|
Updated version of the patch after incorporating Philip's comments
comment:18 by , 12 years ago
For reference, I've attached the updated patch. Should be ready to be committed.
comment:20 by , 12 years ago
Keywords: | review removed |
---|
comment:21 by , 12 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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.
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).