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)

rallypoints_wip_25aug11.patch (136.5 KB ) - added by vts 13 years ago.
Rally point marker lines/focus command WIP patch
rallypoints_wip_26aug11.patch (135.6 KB ) - added by vts 13 years ago.
Updated version of 25aug patch, improved rounded cap rendering on funky terrain
rallypoints_wip_11sep11.patch (131.6 KB ) - added by vts 13 years ago.
Small bugfix in the focus-rally button
rallypoints_06dec11.patch (126.7 KB ) - added by vts 12 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 by JohnDorian, 14 years ago

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

comment:2 by fcxSanya, 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 :)

in reply to:  2 comment:3 by JohnDorian, 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

comment:4 by fcxSanya, 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:

  1. create function, which centers camera on rallypoint, in CGameView class (source\graphics\gameview.cpp) and expose it to scripts;
  2. add button on commands panel (getEntityCommandsList function in binaries\data\mods\public\gui\session_new\utility_functions.js) in case if entity is building
  3. call function from item 1 for command from item 2 (probably in performCommand function from binaries\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:5 by (none), 13 years ago

Milestone: Unclassified

Milestone Unclassified deleted

comment:6 by Andrew, 13 years ago

Milestone: Backlog

in reply to:  4 comment:7 by vts, 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 historic_bruno, 13 years ago

Owner: set to vts

by vts, 13 years ago

Rally point marker lines/focus command WIP patch

comment:9 by vts, 13 years ago

Keywords: review added
Status: newassigned

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 historic_bruno, 13 years ago

Milestone: BacklogAlpha 7

comment:11 by historic_bruno, 13 years ago

Summary: Render marker line between building and rally point[PATCH] Render marker line between building and rally point

comment:12 by historic_bruno, 13 years ago

Keywords: simple review → simple,review

by vts, 13 years ago

Updated version of 25aug patch, improved rounded cap rendering on funky terrain

comment:13 by Philip Taylor, 13 years ago

Milestone: Alpha 7Alpha 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 vts, 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 vts, 13 years ago

Small bugfix in the focus-rally button

comment:15 by Philip Taylor, 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 calls cmpRallyPoint.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 a debug_warn(L"invalid line cap string") and return some default LineCap if it's wrong, to avoid the need for the bool 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 have RenderTexturedOverlayLines iterate over the whole texlines 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 of ICmpPathFinder.h needs to be ICmpPathfinder.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 using g_Renderer. Need to check CRenderer::IsInitialised before doing any graphics stuff there.
  • "LOGERROR(L"Unrecognized LineStartCap value \"%ls\"", xmlLineStartCap);" - should be xmlLineStartCap.c_str() else it'll crash. (GCC warns about these cases). (But see earlier comments about line-cap error-handling.)
  • RecomputeRallyPointPath: the default 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 the ship 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), not Engine.GuiInterfaceCall(...). But actually performCommand already has a entState 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, this IsIgnoreLos 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 an m_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 with minimap=false, and now CameraMoveTo calls it with minimap=true but isn't the minimap, which is confusing. Probably should delete the minimap argument entirely (and always use the true path inside MoveCameraTarget).

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 over indices 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 the LINECAP_SHARP case, to make it clear you didn't just forget the break.

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 of ICmpPlayer.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 for m_TexturedOverlayLines shouldn't have the <.
  • Don't say <a:component/> (that's only meant to be used when there's a type 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 the IsOk checks in Init too, since the schema will guarantee they're all okay.)
  • LineEndCap schema <value>s need indenting.
  • HandleMessage MT_OwnershipChanged and MT_TurnStart should do a break at the end. (If OwnershipChanged is intentionally falling through to the next case, it shouldn't since fallthrough is ugly and confusing - just duplicate the UpdateOverlayLines into the earlier case.)
  • SetPosition, SetDisplayed don't need to bother creating a temporary bool - 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 against INVALID_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.

in reply to:  15 comment:16 by vts, 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 calls cmpRallyPoint.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).

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 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).)

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 of ICmpPathFinder.h needs to be ICmpPathfinder.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 using g_Renderer. Need to check CRenderer::IsInitialised before doing any graphics stuff there.

"LOGERROR(L"Unrecognized LineStartCap value \"%ls\"", xmlLineStartCap);" - should be xmlLineStartCap.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 with minimap=false, and now CameraMoveTo calls it with minimap=true but isn't the minimap, which is confusing. Probably should delete the minimap argument entirely (and always use the true path inside MoveCameraTarget).

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 and MT_TurnStart should do a break at the end. (If OwnershipChanged is intentionally falling through to the next case, it shouldn't since fallthrough is ugly and confusing - just duplicate the UpdateOverlayLines 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 against INVALID_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 Kieran P, 12 years ago

Type: taskenhancement

comment:18 by Kieran P, 12 years ago

Priority: Nice to HaveShould Have

comment:19 by vts, 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 vts, 12 years ago

Attachment: rallypoints_06dec11.patch added

comment:20 by vts, 12 years ago

Resolution: fixed
Status: assignedclosed

Implemented in [10704].

Note: See TracTickets for help on using tickets.