Page MenuHomeWildfire Games

Visual move order indicator
ClosedPublic

Authored by Stan on Jun 5 2017, 3:13 PM.

Details

Reviewers
elexis
fatherbushido
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP20071: Visual move order indicator.
Summary

As borg stated

5- What I'm going to put here seems to be something of no value, but I missed it a few days ago, and I'll expatiate because.

As you can see in the picture, most games have an indicator of where your unit will move, after your click, at 0 a.d we do not have this, I never felt lacking to be honest, but days ago I was having problems in my Mouse, right click, then sometimes it ended up not clicking, so I got lost several times, (there were not few) because I thought I had clicked and had not, as we do not have the indicator I had no way to know. I relied on the audio, okay is a good indicator, but have you stopped to think about possible deaf users? I think an indicator would really be very useful, especially for users with possible hearing problems.
Test Plan

./

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Stan added inline comments.Jun 8 2017, 8:10 AM
binaries/data/mods/public/simulation/components/GuiInterface.js
1044

Sounds fair.

1048

Indeed, else you wouldn't know where you click.

Stan updated this revision to Diff 2470.Jun 8 2017, 8:11 AM
Stan marked 3 inline comments as done.

rename pos to cmpPosition.

Let it error out in case it doesn't exist.

Stan marked 3 inline comments as done.Jun 8 2017, 8:12 AM
leper added inline comments.
binaries/data/mods/public/simulation/components/GuiInterface.js
1056

Missing ;.

binaries/system/Atlas.bat
1

I'm quite sure that this shouldn't be in here.

Stan updated this revision to Diff 2516.Jun 10 2017, 7:14 PM

Fix two oversights.

temple added a subscriber: temple.Jul 24 2017, 6:40 PM

I think there are OOS errors coming from the Timer portion, at least.

(I'm not thrilled about the arrows, maybe because they're in the air? I think I'd prefer something that stays on the ground, like a big circle that shrinks to a small circle around the point, similar to the footprint/selection circles, maybe in the player's color too. But that's just my opinion.)

fatherbushido edited edge metadata.Aug 10 2017, 9:01 AM

I have an issue with the textures. (so the arrows are not textured and the related error is thrown).
I perhaps did bad when applying the patch?

Stan added a comment.Aug 10 2017, 9:23 AM

Uh I think you have to copy them manually. I never got it to work with binaries.

fatherbushido added inline comments.Aug 12 2017, 10:21 AM
binaries/data/mods/public/gui/session/unit_actions.js
1491

I don't know if it's the good folder to use.
special/ seems seems to fit better.

binaries/data/mods/public/simulation/components/GuiInterface.js
1050

Is there any reason for choosing 1100 and not 1000 for example? (not that I think that 1000 is more beautifull than 1100).

binaries/data/mods/public/simulation/templates/other/target_marker.xml
30

I don't know if it's the good folder to use.
special/ seems seems to fit better.

Stan added inline comments.Aug 12 2017, 12:23 PM
binaries/data/mods/public/gui/session/unit_actions.js
1491

Okay I'll move it.

binaries/data/mods/public/simulation/components/GuiInterface.js
1050

That's about the time at a normal turn length where the anim loops twice.

binaries/data/mods/public/simulation/templates/other/target_marker.xml
30

Okay I'll move it.

fatherbushido added inline comments.Aug 12 2017, 12:30 PM
binaries/data/mods/public/simulation/components/GuiInterface.js
1050

So it's related to your animation?
Could you detail a bit more for my own knowledge?

Stan updated this revision to Diff 3090.Aug 12 2017, 12:33 PM

Change the template location.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1846/ for more details.

That's strange, I have now a segfault when testing (which I hadn't before). I hope it's because I didn't do something thing properly. I will try to do a clean compilation and to reapply the patch.

With that warning

tex_codec.cpp(70): Function call failed: return value was -120101 (Unknown error (-120101, 0xFFFFFFFFFFFE2ADB))
Function call failed: return value was -120101 (Unknown error (-120101, 0xFFFFFFFFFFFE2ADB))
Location: tex_codec.cpp:70 (tex_codec_for_header)

and that bt

#9  0x00005556aebe0fc9 in Tex::decode (this=this@entry=0x7ffeb035d8c0, Data=
    std::shared_ptr (count 1, weak 0) 0x0, DataSize=0)
    at ../../../source/lib/tex/tex.cpp:722
#10 0x00005556ae9c8a41 in CTextureConverter::ConvertTexture (
    this=this@entry=0x5556b0365f68, 
    texture=std::shared_ptr (count 3, weak 7) 0x5556cb6c31d0, src=..., 
    dest=..., settings=...)
    at ../../../source/graphics/TextureConverter.cpp:334
#11 0x00005556ae96c705 in CTextureManagerImpl::ConvertTexture (texture=..., 
---Type <return> to continue, or q <return> to quit---
    this=0x5556b0365f20) at ../../../source/graphics/TextureManager.cpp:316
#12 CTextureManagerImpl::MakeProgress (this=0x5556b0365f20)
    at ../../../source/graphics/TextureManager.cpp:381
#13 CTextureManager::MakeProgress (this=<optimized out>)
    at ../../../source/graphics/TextureManager.cpp:663
#14 0x00005556ae64f72d in RendererIncrementalLoad ()
    at ../../../source/main.cpp:267
#15 Frame () at ../../../source/main.cpp:322
#16 RunGameOrAtlas (argc=<optimized out>, argv=<optimized out>)
    at ../../../source/main.cpp:581
#17 0x00005556ae63f067 in main (argc=1, argv=0x7ffeb035f3c8)
    at ../../../source/main.cpp:623

Forget my previous comment.
It's ok after saving the two binaries files.
That's strange that in my last tests, I didn't had a segfault but instead the failing loading texture message.
I guess something changed in the engine code meanwhile.

I was looking at the template part, did you check all entries?
For example, is it better to inherit from special/actors like rallypoints (and adding Ownership) or to template_entity_quasi?
Btw do we really need to see that in fow? (I won't discuss design, just to be sure).
At last, is binaries/data/mods/public/art/textures/skins/skeletal/ the good folder? (I don't really know art folder convention, but it seems that it doesn't contain ui thing)

Stan added a comment.Aug 20 2017, 11:28 AM

I was looking at the template part, did you check all entries? For example, is it better to inherit from special/actors like rallypoints (and adding Ownership) or to template_entity_quasi?

I don't think ownership matters much as it's not really a player entity.

Btw do we really need to see that in fow? (I won't discuss design, just to be sure).

To me, yes. The aim is to be able to see where you click so it doesn't make sense not to see it.

At last, is binaries/data/mods/public/art/textures/skins/skeletal/ the good folder? (I don't really know art folder convention, but it seems that it doesn't contain ui thing)

I had the same concern but this object is actually a skeletal unit so I thought it made sense to have it here.

fatherbushido resigned from this revision.Aug 20 2017, 11:26 PM
In D606#32007, @Stan wrote:

I was looking at the template part, did you check all entries? For example, is it better to inherit from special/actors like rallypoints (and adding Ownership) or to template_entity_quasi?

I don't think ownership matters much as it's not really a player entity.

Stan, I am not your father nor your teacher, but you use that in your code and you even answer to leper about that...

Well, template can perhaps be improved but patch works as expected. Nonetheless, I won t finish my review because I don t feel myself of good advice for judging such a ui feature. Don t take that personnaly.

Stan added a comment.Aug 20 2017, 11:45 PM

Stan, I am not your father nor your teacher, but you use that in your code and you even answer to leper about that...

@fatherbushido I'm not sure I get all you mean by that. However yes I use it and I have answered a bit too fast.

Well, template can perhaps be improved but patch works as expected. Nonetheless, I won t finish my review because I don t feel myself of good advice for judging such a ui feature. Don t take that personnaly.

Okay no problem as long as you say why ;)

I don t feel myself of good advice for judging such a ui feature

With regards to the actor, I think this is one of the rare cases where a placerholder item would be justified and then ask Enrique. He also did the catafalque request once he (1) was available, (2) saw that the code to use it was in place and (3) it is only one model. LordGood, Pureon, wowgetoffyourcellphone and Lion.Kanzen and whomever I forgot might be active too. Finally we can ask the entire forum to try to create a better one or visit some websites with freely licensed models.

In D606#25170, @leper wrote:

If I give 3 commands I'd expect to see 3 markers.

Agree, would appear nicer.

binaries/data/mods/public/art/animation/other/target_marker.dae
6

If you ever wanted to put your name somewhere

binaries/data/mods/public/gui/session/unit_actions.js
1488

ok. (We can add a new GUI function once we have a second type that reuses the same GUI Interface function with a different hardcoded template)

1491

Dunno about the folder either.
That string should be a global variable, so that modders can change it by adding a new file instead of adding a famous copy of the public mod file.

binaries/data/mods/public/simulation/components/GuiInterface.js
1065

So is ownership needed for fogging issues or not?

(There are number of loops that iterate through owned entities, but local entities must be excluded (1. by definition and 2. because otherwise stuff would have blown up already on a large scale))

(There is also stuff like)

<Visibility>
  <RetainInFog>true</RetainInFog>
1068

Adding serialized timers for non-serialized entities for fun & profit?
Use a GUI timer that calls the remove method.

binaries/data/mods/public/simulation/templates/special/target_marker.xml
2 ↗(On Diff #3090)

Someone should review this parent

Stan updated this revision to Diff 3272.Aug 21 2017, 10:16 PM
Stan marked 3 inline comments as done.

Agree, would appear nicer.

It's already done.

Just tested the uploaded version with the simulation timers. That indeed yields an OOS as expected. It's important to notice that the animation is not always played exactly twice, but sometimes 2.5 times, sometimes 3.5 times before the arrows disappear.
I don't expect that this can be solved with (local) simulation timers nor GUI timers, because it is tied to the renderer. Also it should be played exactly once.

elexis requested changes to this revision.Aug 22 2017, 12:42 AM

Change the actor somehow to stop showing the animation after playing it once.
We either have to add support for local simulation timers or GUI timers with arguments. Both are easy and could be done in a separate patch.
Whatever timer is used can delete the animation some time after it played.

This revision now requires changes to proceed.Aug 22 2017, 12:42 AM

Build has FAILED

Link to build: http://jw:8080/job/phabricator/1901/
See console output for more information: http://jw:8080/job/phabricator/1901/console

Stan updated this revision to Diff 3278.Aug 22 2017, 1:19 AM
Stan edited edge metadata.

Add an anim that disappear below ground for a time so that it doesn't popup
Add global variable.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1902/ for more details.

elexis requested changes to this revision.Aug 24 2017, 8:48 PM

I think we need a local simulation timer for this.

If the gamespeed is reduced to 0.1, then the animations are played 10 times slower.
So the 1100ms would become 11s and the timer runs out too soon.
The user could set any gamespeed, even 0.00001 if he wanted to.

If the walk order is sent, the game is paused, then GUI timer runs out and calls the marker deletion on the same turn. So it's probably never seen in that case.

So a simulation timer would fit better. We must add support for local timers in a separate patch to not become OOS by using the current, synced simulation timer.

There is SetAnimationSyncRepeat in IcmpVisual.h:

Adjust the speed of the current animation, so it can match simulation events

So it sounds a lot like the GUI interface would be able to add the local entity, set that animation, then read the animation duration,
in which case we could nuke the hardcoded 1100ms.

(Since the timer is only executed once per turn, we still have to use the hiding of the cursor inside the model/animation cheat.)

This revision now requires changes to proceed.Aug 24 2017, 8:48 PM

Adding a decay component to the template should make the timer entirely obsolete. That's the component that also deletes local corpse entities.
It consumes a decay time, so the 1100ms would go into the template, which is much better than in the sim code.

Stan updated this revision to Diff 3320.Aug 26 2017, 3:33 PM

Remove timers and use decay instead.

Stan updated this revision to Diff 3321.Aug 26 2017, 3:37 PM

Remove ownership set, as it's not used.

Stan updated this revision to Diff 3322.Aug 26 2017, 4:05 PM

Fix vanishing, and revert anim to the non tweaked one.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1917/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1918/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1919/ for more details.

elexis accepted this revision.Aug 28 2017, 11:08 AM

(The one testing this patch must download the png files manually from Stans comment because Phabricator.)

Serialization: Correct now (as in nothing influencing a serialized value) without having to add a GUI or local simulation timer. Nothing has to be deserialized (who cares about half a target-marker animation).

Template:

  • <SinkingAnim> no, thanks
  • <SinkRate>1000000</SinkRate> because it should be hidden instantaneously
  • <SinkAccel>0</SinkAccel> because why would it need to accelerate. we can save some cycles here
  • With regards to trailing zeroes in the numbers, I'd either avoid them or keep it consistent with the rest of the XML files.
  • Directory: That directory is ok. It's ok that the full path is specified in the session IMO. In some situation we might want to use a different special directory.

Art: looks good to me. If someone wants to propose something better, there is the chance to do it beforehand or afterwards. I assume it will get more attention once it's committed.

GUIInterface:

  • (Instead of calling AddTargetMarker in unitActions, one could add the local entities in Commands.js directly. But that doesn't sound exactly right, because it's a local entity, so it shouldn't influence the command objects. Also there might be any situation where we want to use this method again.)
  • (The new GUI interface command can be used to spawn any template, for example local civic centers and getting out of sync immediately. I don't recall any other GUI Interface call that can do that, but there are enough JS+XML ways to get oneself OOS.)

Session:

  • Agree with the wording Target Marker (because for all unit commands, there is the variable in commands.js called target and target is a common military term, so it's easily understandable for both coders and players).
  • The object should eventually contain all markers that are used for the different unit orders,i.e

g_TargetMarkers containing "move": "...templatename", "attack": "templatename...", ...

  • Rename "template" to "move". (The key "move" should either correlate with the strings in commands.js or with the ones from unitActions. Considering that we're in the GUI, using the keys from unitActions is more comfortable.

Unfortunately we can't make the target marker template names properties of unitActions and do the guiinterface call outside of that object, because not each unitaction might have a target (remove-guard) or would want it displayed (set-rallypoint) and because we'd have to return the target position in those function (bit meh). However g_TargetMarkers should be part of unit_actions.js because its primary use lies in that file.)

Future patches:

  • Option to disable it
  • Displaying the target markers in observermode if the follow-player option is enabled (I'd be happy to write the patch). This would allow visualizing the wesono dance for example.
  • The special directory could be sorted a bit. For example the player templates could go into a separate folder.

So I don't like to do this but I have to click accept (assuming var rename and template update).

This revision is now accepted and ready to land.Aug 28 2017, 11:08 AM

Template:

  • SilhouetteOccluder can be false too
  • The OverlayRenderer is not needed
  • Selectable and properties are needed. (Could or could not improve features for that component.)
Stan updated this revision to Diff 3360.Aug 29 2017, 1:04 AM

Rename "template" to "move". (The key "move" should either correlate with the strings in commands.js or with the ones from unitActions. Considering that we're in the GUI, using the keys from unitActions is more comfortable.

Done

So I don't like to do this but I have to click accept (assuming var rename and template update).

Done

SilhouetteOccluder can be false too
The OverlayRenderer is not needed
Selectable and properties are needed. (Could or could not improve features for that component.)

Done

Fixed missing endline in template removed the extra stuff, and fixed anim time which apparently was broken when tweaking the values to what you asked.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/1929/ for more details.

Itms awarded a token.Aug 29 2017, 10:55 AM

Good job.

It would be nice if Stan gives sooner or later a precise input about what is needed in that template.

final template is:

<Entity>
  <AIProxy/>
  <Ownership/>
  <Footprint>
    <Circle radius="2.0"/>
    <Height>8.0</Height>
  </Footprint>
  <Decay>
    <Active>true</Active>
    <SinkingAnim>false</SinkingAnim>
    <DelayTime>0.5</DelayTime>
    <SinkRate>1000000</SinkRate>
    <SinkAccel>0.0</SinkAccel>
  </Decay>
  <Position>
    <Altitude>0</Altitude>
    <Anchor>upright</Anchor>
    <Floating>true</Floating>
    <TurnRate>6.0</TurnRate>
  </Position>
  <Selectable>
    <EditorOnly/>
    <Overlay>
      <Outline>
        <LineTexture>outline_border.png</LineTexture>
        <LineTextureMask>outline_border_mask.png</LineTextureMask>
        <LineThickness>0.4</LineThickness>
      </Outline>
    </Overlay>
  </Selectable>
  <Visibility>
    <RetainInFog>true</RetainInFog>
    <AlwaysVisible>true</AlwaysVisible>
    <Corpse>false</Corpse>
    <Preview>true</Preview>
  </Visibility>
  <VisualActor>
    <SilhouetteDisplay>false</SilhouetteDisplay>
    <SilhouetteOccluder>false</SilhouetteOccluder>
    <VisibleInAtlasOnly>false</VisibleInAtlasOnly>
    <Actor>special/target_marker.xml</Actor>
  </VisualActor>
</Entity>

To compare, special/rallypoint

<Entity>
  <Position>
    <Altitude>0</Altitude>
    <Anchor>upright</Anchor>
    <Floating>false</Floating>
    <FloatDepth>0.0</FloatDepth>
    <TurnRate>6.0</TurnRate>
  </Position>
  <Visibility>
    <RetainInFog>true</RetainInFog>
    <AlwaysVisible>true</AlwaysVisible>
    <Corpse>false</Corpse>
    <Preview>false</Preview>
  </Visibility>
  <Vision>
    <Range>0</Range>
  </Vision>
  <VisualActor>
    <Actor>props/special/common/waypoint_flag.xml</Actor>
    <SilhouetteDisplay>false</SilhouetteDisplay>
    <SilhouetteOccluder>false</SilhouetteOccluder>
    <VisibleInAtlasOnly>false</VisibleInAtlasOnly>
  </VisualActor>
</Entity>

or special/actor

<Entity>
  <Position>
    <Altitude>0</Altitude>
    <Anchor>upright</Anchor>
    <Floating>false</Floating>
    <FloatDepth>0.0</FloatDepth>
    <TurnRate>6.0</TurnRate>
  </Position>
  <Visibility>
    <RetainInFog>true</RetainInFog>
    <AlwaysVisible>false</AlwaysVisible>
    <Corpse>false</Corpse>
    <Preview>false</Preview>
  </Visibility>
  <Vision>
    <Range>0</Range>
  </Vision>
  <VisualActor>
    <SilhouetteDisplay>false</SilhouetteDisplay>
    <SilhouetteOccluder>false</SilhouetteOccluder>
    <VisibleInAtlasOnly>false</VisibleInAtlasOnly>
  </VisualActor>
</Entity>
Stan updated this revision to Diff 3367.Aug 29 2017, 12:58 PM

Thanks fatherbushido for looking into it.

This patch updates the actor to be more like the rally point.

Decay needed for auto disappearing after a certain time, sinktime is big to make sure it's instantaneous.
sinkaccel is mandatory even though it's set to

Position (Floating) -> Else it wouldn't work for ships.

Selectable -> Was not needed if it's not a preview. Since rally points are not it doesn't makes sense that the target marker is.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jenkins-master:8080/job/phabricator/1930/ for more details.

elexis accepted this revision.Aug 29 2017, 6:57 PM

Thanks fatherbushido, besides Stan I missed some components in my testing like AIProxy and Footprint as well. The new template is much better. (Still don't know why the previous iteration complained if Selectable was removed, but good that the current one doesn't need it).

The new patch still inherits Vision from actors without any noticeable reason for both actor templates and this template. It seems to be an oversight in https://trac.wildfiregames.com/changeset/16022/ps/trunk/binaries/data/mods/public/simulation/templates/special/actor.xml following https://trac.wildfiregames.com/changeset/9184 So I propose to clean that separately. Everything specified in the template marker is needed, everything unneeded that is inherited (that one component that is) is most likely not wanted in the actor as well. So this time the completeness test passed hopefully.

For observermode, we will have to copy the logic from unit_actions.js to messages.js in playercommand, because we want targets for players to appear instantaneously, rather than when the command is executed after 2 turns (so it can't be unified in the simulation command execution stack).

From a player point of view, I must say I don't really need it, because I see the mouse cursor as an indication for the target and the marker disappears before I move the mouse somewhere else. But it's a great feature to have IMO and we should add more markers for attack commands and so forth. It should become optional as stated before.

Eventually DrawTargetMarker will need the command type, but thats not needed now, so not going to add it. g_TargetMarker could also have "patrol": "special/target_marker" in the global, but not doing so for the same reason (useless duplication).

unit_actions.js is complete, because every other command receives an entity ID as a target and we want to use a different marker for those actions rather than pointing on the ground.

move,attack-move,capture,attack,patrol,heal,build,repair,gather,returnresource,setup-trade-route,garrison,guard,remove-guard,set-rallypoint,unset-rallypoint,none

The marker is visible in the fog of war - but do we want to see the marker in the shroud of darkness as well? It's currently hidden. I assume we don't want to (so that the terrain can't be cheated).

binaries/data/mods/public/gui/session/unit_actions.js
1491

g_TargetMarkers to the top of the file. Nuke that "default" because all GUI variables are defaults.

1497

unused variable

binaries/data/mods/public/simulation/components/GuiInterface.js
1039

ent seems a more common name than templateMarker, though the latter is legit too.

1041

Either don't return anything in both cases or return something in both cases (as in explcitily undefined).

and thanks for the patch!

This revision was automatically updated to reflect the committed changes.