Opened 14 years ago
Closed 12 years ago
#524 closed enhancement (fixed)
[PATCH] Render marker line between building and rally point
Reported by: | Philip Taylor | Owned by: | vts |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 8 |
Component: | Core engine | Keywords: | |
Cc: | Patch: |
Description
When a building's rally point is a long way away, it's easy to lose the little flag marker. It'd probably be nice if we could display some kind of line between the building and rally point once you select the building, so you can follow it visually to find the end.
I don't know what the line should look like. It shouldn't be too distracting (people will often select buildings for other reasons and not care about the rally point), but it shouldn't be too subtle to notice.
Attachments (4)
Change History (24)
comment:1 by , 14 years ago
follow-up: 3 comment:2 by , 14 years ago
Hi JohnDorian
. Probably the DisplayRallyPoint
function from binaries\data\mods\public\simulation\components\GuiInterface.js
it is that what you need. I think you should use (or implement by yourself) some graphical code in C++ game engine.
Your language is quite clear for me (English isn't my native language too :)
comment:3 by , 14 years ago
Replying to fcxSanya:
I think you should use (or implement by yourself) some graphical code in C++ game engine.
Hi Sanya
Thanks for your answer!
Well... I'm not that bad at programming C++ (and simple OpenGL) but... after playing (or testing) this game I wanted to help, and so I really don't know, where I could implement this...
You know, I don't know JS at all...
Thanks, JD
follow-up: 7 comment:4 by , 14 years ago
We discussed in IRC another solution, which allows to find rally point flag: we can center screen on flag by clicking on some button in buildings interface. Probably it should be binded on double-click on "set rally point flag" button, which also should be added :)
This option seems better for me, because I have not idea how to draw line, which can blend with interface and will not looks ugly.
Also this functionality is similar to camera following mode in engine side implementation (see #56 and r8190), so I think we should:
- create function, which centers camera on rallypoint, in
CGameView
class (source\graphics\gameview.cpp
) and expose it to scripts; - add button on commands panel (
getEntityCommandsList
function inbinaries\data\mods\public\gui\session_new\utility_functions.js
) in case if entity is building - call function from item 1 for command from item 2 (probably in
performCommand
function frombinaries\data\mods\public\gui\session_new\input.js
)
P.S. JohnDorian
, I think IRC is good place for fast development discussions, so you can join the channel #0ad
on QuakeNet
if you want more active communications.
comment:6 by , 13 years ago
Milestone: | → Backlog |
---|
comment:7 by , 13 years ago
Currently working on getting this done. I have a rudimentary overlay line working that grabs a long-range path to the rally point and reduces tile zig/zags a bit to produce straighter lines. Philip is currently working on some spline interpolation and pretty textured lines for the territory outlines, which can probably be reused for rendering pretty rally point paths as well.
I also added a command button to make the camera jump to the rally position, as per fcxSanya's suggestion above. It currently works on single-click rather than double-click since I figure that for now right-click is fast enough (both on terrain and on the minimap) that a separate button to set rally points isn't really needed. I used a basic crosshair icon for the button for now, so the art department will probably have to look into getting a fancier-looking icon for that.
comment:8 by , 13 years ago
Owner: | set to |
---|
by , 13 years ago
Attachment: | rallypoints_wip_25aug11.patch added |
---|
Rally point marker lines/focus command WIP patch
comment:9 by , 13 years ago
Keywords: | review added |
---|---|
Status: | new → assigned |
As requested on IRC, here's a git patch showcasing the WIP on the rally point marker lines (including dashed segments for areas under the SoD), as well as the rally point focus command button.
It also includes some improvements to existing code, notably to enable the spline interpolation code to deal with open lines, to enable STexturedOverlayLines to remain always visible (even if under SoD), and a generic SDashedLine struct.
comment:10 by , 13 years ago
Milestone: | Backlog → Alpha 7 |
---|
comment:11 by , 13 years ago
Summary: | Render marker line between building and rally point → [PATCH] Render marker line between building and rally point |
---|
comment:12 by , 13 years ago
Keywords: | simple review → simple,review |
---|
by , 13 years ago
Attachment: | rallypoints_wip_26aug11.patch added |
---|
Updated version of 25aug patch, improved rounded cap rendering on funky terrain
comment:13 by , 13 years ago
Milestone: | Alpha 7 → Alpha 8 |
---|
Don't have time to look at this in great detail now (nor soon) (and haven't tried compiling or running), but a few general non-comprehensive comments:
In general it looks great :-)
overlayline.fp
: Shouldn't it still use the alpha from the base texture, not 1, so the texture can choose to do nice blending at the edges?
Overlay.cpp
: Instead of
if (str.compare(L"flat") == 0)
use
if (str == L"flat")
to be conventional.
ScriptFunctions.cpp
: Looks like you could just rename the MoveCameraTarget2D
definition instead of having a comment about having inconsistent names.
Vector2D.h
: Should use cosf
, sinf
(cos
etc is for doubles).
OverlayRenderer.cpp
: Should use the same vertex buffer for all the data, instead of having a separate ones for caps, and make them all GL_TRIANGLES
(don't bother with quads or strips or fans since they're a waste of time and just make the code messier and less efficient).
OverlayRenderer.cpp
: Should add the cassert
back in, and add some padding to the struct to make it a power-of-two in size.
ScriptInterface.cpp
: Why add logr
? (If it's just for debugging then warn
should work just as well, and avoids adding more code (and I'm generally in favour of minimising lines of code). Also logr
seems an ugly name but I'm generally in favour of avoiding naming debates :-) )
CCmpRallyPoint.cpp
: In the schema, "
is a double-quote; you probably want '
. But it's probably not worth mentioning the texture mapping in there anyway, because people writing XML files won't care about that implementation detail and it'll just clutter the documentation.
CCmpRallyPoint.cpp
: In the schema, the a:help
values should not end with a "." (since the documentation-generator adds one automatically).
Code like
LOGERROR(L"Unrecognized LineStartCap value \"%s\"", xmlLineStartCap);
needs to be
LOGERROR(L"Unrecognized LineStartCap value \"%ls\"", xmlLineStartCap.c_str());
else GCC will explode.
CCmpRallyPoint.cpp
: (I got too hungry and didn't read the rest of the file :-( )
CCmpSelectable.cpp
: Presumably shouldn't be in this patch.
Render.cpp
: Instead of all the dashed line stuff, wouldn't it be easier to make the line-renderer alter the V coordinate at some constant rate along the line, and then use a dashed texture?
EngineScriptConversions.cpp
: I don't think we should add RallyPointPosition
since it's redundant and likely to lead to confusion; better to stick consistently with CFixedVector2D
for all 2D vectors. (Arguably we should change CFixedVector2D
to always be x/z, not x/y, but that's a separate issue and should be fixed everywhere rather than just for rally points.)
comment:14 by , 13 years ago
All valid points, except perhaps the dashed texture. As discussed on IRC, using a dashed texture significantly reduces flexibility in the spacing of the dashes and gaps, and more importantly, it would require the end caps to be hardcoded in the texture, which would probably end up getting distorted if scaling is applied to change spacing.
I've incorporated your comments so far in a new version of the patch. I did keep GL_QUAD_STRIP rendering for the main line, while at the same time keeping all data in a single VB and reducing lines of code. Turns out it's trivial to combine GL_TRIANGLES for the caps and GL_QUAD_STRIP for the main line in the same VB.
by , 13 years ago
Attachment: | rallypoints_wip_11sep11.patch added |
---|
Small bugfix in the focus-rally button
follow-up: 16 comment:15 by , 12 years ago
Keywords: | simple review removed |
---|
Some more detailed comments on the latest version:
GuiInterface.js
- The input to
GuiInterface
is not network-synchronised, so it mustn't affect the simulation state (i.e. the data that is serialised and can affect the behaviour of the rest of the simulation) else it'll cause out-of-sync errors.DisplayRallyPoint
callscmpRallyPoint.SetPosition
, which will affect the state, so it will break.
Instead, the displayed rally point position (which updates immediately when the user clicks) needs to be independent of the real simulation-affecting rally point position (only updated by Commands.js
).
(One possibility would be to keep the old scripted RallyPoint
for the simulation behaviour, and rename CCmpRallyPoint
to something like CCmpRallyPointRenderer
for the non-synchronised purely graphical behaviour. As a bonus, cmpRallyPoint.GetPosition
could go back to returning undefined
when it's unset, avoiding the need for the separate IsSet
function (and avoiding the potential for unnoticed bugs when forgetting to call IsSet
).)
Overlay.cpp and related places
- Since the XML schema guarantees the line-caps will be one of the expected strings, the only possible error cases will be engine bugs, so it doesn't seem worth having fancy error-handling code. Just have
StrToLineCap
do adebug_warn(L"invalid line cap string")
and return some defaultLineCap
if it's wrong, to avoid the need for thebool
return value and all the checks for it.
OverlayRenderer.cpp
- I still think there's no value in using quad strips - better to just make everything triangles, then it can be drawn in a single
glDrawElements
call (which is better for performance) and it shouldn't be necessary to maintain all the cap index offsets/counts (which is better for simplicity). - Instead of building the
alwaysVisibleTexLineIndices
/etc vectors, it'd be better just to haveRenderTexturedOverlayLines
iterate over the wholetexlines
and skip the ones with the wrong visible flag. (Building vectors means memory allocation, which is expensive compared to looping twice - it's good to avoid allocations in the per-frame rendering code when that's not too inconvenient.)
CCmpRallyPoint.cpp
#include
ofICmpPathFinder.h
needs to beICmpPathfinder.h
(lowercase F).- Needs to deal with serialization properly, else it'll break saved games. (I think it just requires
m_RallyPoint
to be serialized, unless (per an earlier comment) that position is moved to a scripted component or something. The graphics state doesn't need serializing, since no entities will be selected after loading a game.) - If the game is run without graphics,
Init
will probably crash when usingg_Renderer
. Need to checkCRenderer::IsInitialised
before doing any graphics stuff there. - "
LOGERROR(L"Unrecognized LineStartCap value \"%ls\"", xmlLineStartCap);
" - should bexmlLineStartCap.c_str()
else it'll crash. (GCC warns about these cases). (But see earlier comments about line-cap error-handling.) RecomputeRallyPointPath
: thedefault
passability/cost classes won't be right for e.g. ships. Maybe we should add extra fields to the<RallyPoint>
XML so docks can select theship
passability class instead. (We shouldn't have any buildings that train multiple units of incompatible types, because that would be crazy.)
More trivial/stylistic things:
input.js
- Use
GetEntityState(entity)
, notEngine.GuiInterfaceCall(...)
. But actuallyperformCommand
already has aentState
so you shouldn't re-get the state at all. - If
focus-rally
somehow gets called on a building with no rally point (maybe we'd add a hotkey that people could press even when there's no focus-rally button in the UI), maybe it'd make sense to move the camera to the building position (as in the!entState.rallyPoint.position
case) rather than entirely ignoring the command.
unit_commands.js
- The "
// use item.tooltip
..." comment seems entirely redundant with the code itself, so I'd prefer to remove it. - "
if(entState.rallyPoint)
" - should have space before "(
".
overlayline.fp
- I generally prefer having all the
TEMP
etc declarations at the top of the file (even if they'll be unused in some#ifdef
paths), but I don't know if there's really any common conventions for ARB assembly. - Should start sentences in comments with capital letters, else it gets a bit confusing to read.
GuiInterface.js
- "
{x: cmd.x, y:cmd.z}
" - should be written like "{"x": cmd.x, "y": cmd.z}
" for consistency with the conventions which really ought to be documented somewhere but aren't :-(
Commands.js
- Same with "
{x: cmd.x, y:cmd.z}
".
template_structure.xml
- Should indent with spaces, not tabs.
ShaderProgramFFP.cpp
- Since the ARB shaders do
#ifdef
instead of#if
, thisIsIgnoreLos
should similar just check the thing is defined and not check its value. - Rather than storing
m_Defines
and repeatedly parsing it, it's probably cleaner to just store anm_IgnoreLos
and have the constructor set it appropriately. - "... the previous behaviour before ignore_los was added was ..." - I generally don't like having historical notes in the source code since they should be irrelevant to the current state (the code should make sense by itself) and they're just confusing to anyone who's not familiar with the history of the code. So I'd get rid of that comment entirely; it's not unusual to reset state that wasn't set so I don't think it needs commenting at all.
ScriptFunctions.cpp
- We never call
MoveCameraTarget
withminimap=false
, and nowCameraMoveTo
calls it withminimap=true
but isn't the minimap, which is confusing. Probably should delete theminimap
argument entirely (and always use thetrue
path insideMoveCameraTarget
).
OverlayRenderer.cpp
- "The drawing mode ... is written to @p drawModeOut" - that argument doesn't appear to exist.
- "handed to the rendered" - typo.
- In
RenderTexturedOverlayLines
: probably nicer to iterate overindices
with "for (size_t i = 0; ...)
" since STL iteration is disgustingly verbose and is unnecessary for vectors. CreateLineCap
: Should add an explicit " fall through" comment at the end of theLINECAP_SHARP
case, to make it clear you didn't just forget thebreak
.
RenderModifiers.h
- Delete
RenderModifierRenderer
.
CCmpRallyPoint.cpp
- "
#include "simulation2/components/..."
" would ideally be in alphabetical order. (I was going to say that's probably exceedingly pedantic, but then I realised it would highlight the duplicated inclusion ofICmpPlayer.h
so actually it's vaguely useful :-) ) - Shouldn't have a constructor/destructor; do that work in
Init()
/Deinit()
instead. (Mostly for consistency with other components, and I think keeping all the init code together makes it easier to follow and to see that no fields were forgotten.) ///<
comment form_TexturedOverlayLines
shouldn't have the<
.- Don't say
<a:component/>
(that's only meant to be used when there's atype
attribute). - Don't put a "." at the end of
a:help
attributes. (But do keep it on the<a:help>
element). (The documentation-generating script adds a "." by itself.) - Don't make the elements optional in the schema, and don't set defaults in
Init()
- it's better for all the data to be explicit in the XML files (especially if it only needs to be specified in a small number of XML files, and is inherited by the rest, which I think is the case here). That makes it easier for external tools to process the data files, and for artists to tweak all the settings, and simplifies the code. (Get rid of all theIsOk
checks inInit
too, since the schema will guarantee they're all okay.) LineEndCap
schema<value>
s need indenting.HandleMessage
MT_OwnershipChanged
andMT_TurnStart
should do abreak
at the end. (IfOwnershipChanged
is intentionally falling through to the next case, it shouldn't since fallthrough is ugly and confusing - just duplicate theUpdateOverlayLines
into the earlier case.)SetPosition
,SetDisplayed
don't need to bother creating a temporarybool
- just do like "if (pos != m_RallyPoint) { m_RallyPoint = pos; Recompute... }
".- "Removes waypoints from m_rallyPointPath" - I don't think
m_rallyPointPath
exists. SetMaxAnisotropy
should probably be 2, not 8, to match the territory borders unless there's a reason they ought to differ. (Later it should be a configurable value for performance/quality.)CreateMarker
:AllocateNewLocalEntity
always returns a valid ID, you don't need to check againstINVALID_ENTITY
.- " TODO: is this the right one to use?" - I think it is.
UPDATE_BASENODE_XYZ
is only three lines and only used twice, so I don't think the deduplication is worth the ugliness.- There's some "
if(...
" with insufficient spaces.
CCmpTerritoryManager.cpp
- Why the 7s in the debug display?
- "
for( size_t
" - wrong spacing.
Render.cpp
- Shouldn't need to bother with
fSegmentSamples
, just use(float)segmentSamples
where it's needed.
Indentation needs fixing in ShaderProgramFFP.cpp
, OverlayRenderer.cpp
. (I presume they were left unfixed intentionally to keep the diff cleaner, which is helpful). There's trailing whitespace after comments in various places, which it'd be marginally nice to remove.
comment:16 by , 12 years ago
Replying to Philip:
GuiInterface.js
The input to
GuiInterface
is not network-synchronised, so it mustn't affect the simulation state (i.e. the data that is serialised and can affect the behaviour of the rest of the simulation) else it'll cause out-of-sync errors.DisplayRallyPoint
callscmpRallyPoint.SetPosition
, which will affect the state, so it will break.
Instead, the displayed rally point position (which updates immediately when the user clicks) needs to be independent of the real simulation-affecting rally point position (only updated byCommands.js
).
My apologies, I was not aware of this restriction. As discussed on IRC, it might be useful to make the comment in GuiInterface.js
that warns for this more explicit, or even create a separate high-level documentation page of the interaction between GUI scripts and GuiInterface
and network commands and other simulation code.
(One possibility would be to keep the old scripted
RallyPoint
for the simulation behaviour, and renameCCmpRallyPoint
to something likeCCmpRallyPointRenderer
for the non-synchronised purely graphical behaviour. As a bonus,cmpRallyPoint.GetPosition
could go back to returningundefined
when it's unset, avoiding the need for the separateIsSet
function (and avoiding the potential for unnoticed bugs when forgetting to callIsSet
).)
I think that would probably turn out to work well, and also it helps to cleanly separate the rendering-related code from the rally point bookkeeping, which are now sort of bunched together. I never was very happy with the need for the call to IsSet
anyway.
OverlayRenderer.cpp
I still think there's no value in using quad strips - better to just make everything triangles, then it can be drawn in a single
glDrawElements
call (which is better for performance) and it shouldn't be necessary to maintain all the cap index offsets/counts (which is better for simplicity).
I have far less experience with OpenGL performance issues than you do, so I will defer to your judgement. I really only kept the quads in there because it was quicker to bring it down from quads-only to quads-plus-triangles than all the way to triangles-only.
CCmpRallyPoint.cpp
#include
ofICmpPathFinder.h
needs to beICmpPathfinder.h
(lowercase F).
Woops. Yay for developing on Windows.
Needs to deal with serialization properly, else it'll break saved games. (I think it just requires
m_RallyPoint
to be serialized, unless (per an earlier comment) that position is moved to a scripted component or something. The graphics state doesn't need serializing, since no entities will be selected after loading a game.)
I was aware of that at the time of writing, but I didn't know how to properly serialize data at the time and I guess I forgot to look at it again before submitting. But indeed, if rally points are split into the basic JS component and a separate renderer, then this would be automatically handled by the JS component.
If the game is run without graphics,
Init
will probably crash when usingg_Renderer
. Need to checkCRenderer::IsInitialised
before doing any graphics stuff there.
"
LOGERROR(L"Unrecognized LineStartCap value \"%ls\"", xmlLineStartCap);
" - should bexmlLineStartCap.c_str()
else it'll crash. (GCC warns about these cases). (But see earlier comments about line-cap error-handling.)
Nice catches! I really should start compiling on *NIX ...
More trivial/stylistic things:
input.js
If
focus-rally
somehow gets called on a building with no rally point (maybe we'd add a hotkey that people could press even when there's no focus-rally button in the UI), maybe it'd make sense to move the camera to the building position (as in the!entState.rallyPoint.position
case) rather than entirely ignoring the command.
Hmn, it should already be doing that. Not sure what happened there that caused it to break (or maybe I never included that change in the patch). Will investigate.
overlayline.fp
I generally prefer having all the
TEMP
etc declarations at the top of the file (even if they'll be unused in some#ifdef
paths), but I don't know if there's really any common conventions for ARB assembly.
A matter of convention I suppose. I moved it from the top of the file to within the ifdef branch to play it safe in case shader compilers can't tell the TEMPs aren't being used (for whatever reason), but I'm willing to convert either way.
ShaderProgramFFP.cpp
"... the previous behaviour before ignore_los was added was ..." - I generally don't like having historical notes in the source code since they should be irrelevant to the current state (the code should make sense by itself) and they're just confusing to anyone who's not familiar with the history of the code. So I'd get rid of that comment entirely; it's not unusual to reset state that wasn't set so I don't think it needs commenting at all.
Ok. I put that comment there to give an indication to anyone wondering if why possibly unset state is being reset, but if it's not that uncommon then I'll leave it out.
ScriptFunctions.cpp
We never call
MoveCameraTarget
withminimap=false
, and nowCameraMoveTo
calls it withminimap=true
but isn't the minimap, which is confusing. Probably should delete theminimap
argument entirely (and always use thetrue
path insideMoveCameraTarget
).
Ok, will go ahead and make that change.
CCmpRallyPoint.cpp
Shouldn't have a constructor/destructor; do that work in
Init()
/Deinit()
instead. (Mostly for consistency with other components, and I think keeping all the init code together makes it easier to follow and to see that no fields were forgotten.)
I agree, moving the field initializations from the constructor to Init() where it says // implicit constructor defaults
makes complete sense. Not sure why I didn't think of that before.
HandleMessage
MT_OwnershipChanged
andMT_TurnStart
should do abreak
at the end. (IfOwnershipChanged
is intentionally falling through to the next case, it shouldn't since fallthrough is ugly and confusing - just duplicate theUpdateOverlayLines
into the earlier case.)
Oh that's bad. There's definitely supposed to be a break there. Never ran into the bug because I never tested ownership changes since I didn't know how to do that at the time. Sorry, will fix.
SetMaxAnisotropy
should probably be 2, not 8, to match the territory borders unless there's a reason they ought to differ. (Later it should be a configurable value for performance/quality.)
I've found that anything lower than 8 causes ugly blurring when viewing long straights of marker line head-on from a sharp angle -- at least, for the thick black-and-white textures I used when I first chose 8x. In the meantime I've arbitrarily changed it to a thinner blue line, and now the difference of 8x vs 2x is less obvious but still there. I suppose the final value would depend on the way the lines are styled and what is deemed an acceptable visual quality for extreme viewing angles.
(I would also like to reiterate the point that the current style of the rally marker lines is completely arbitrary. Would be nice if the art team could step in and decide on a pretty look that's unobtrusive to the player while still clear enough to visually lead a player to the rally point flag).
CreateMarker
:AllocateNewLocalEntity
always returns a valid ID, you don't need to check againstINVALID_ENTITY
.
I know, but its implementation has a comment stating that handling ID overflow is a TODO, so I didn't really want to rely on it always returning a valid ID for future compatibility.
CCmpTerritoryManager.cpp
Why the 7s in the debug display?
There's no particular reason for it to be 7; at one point during debugging I just needed the X first and last nodes to be colored differently so I could see where a territory border started and closed on itself. I just picked X = 7. Should probably have added a comment for that.
comment:17 by , 12 years ago
Type: | task → enhancement |
---|
comment:18 by , 12 years ago
Priority: | Nice to Have → Should Have |
---|
comment:19 by , 12 years ago
Attaching the modified version of the patch as per Philip's comment. If possible, I'd like to have it reviewed quickly (as in a short look) to make sure I corrected some issues correctly -- particularly the issue about non-synchronized input to GuiInterface.js. Other than that, I think it's ready to be committed.
by , 12 years ago
Attachment: | rallypoints_06dec11.patch added |
---|
Hi, I could try this, if somebody tells me, in which file (or at least folder) I can find the render of the rally point.
Hope you understand me (English is not my mother tongue...)
JD