Page MenuHomeWildfire Games

Ignore few Ownership changes in Selectable and RallyPointRenderer and properly support capturing for the latter
ClosedPublic

Authored by elexis on Aug 9 2017, 1:39 PM.

Details

Summary

As noticed in D687, the Selectable and RallyPointRenderer component execute some unneeded code

  • if the entity has just been created and then received ownership right after that
    • not needed for Selectable because the overlay is created upon selection
    • not needed for RallyPointRenderer because the entity cannot start with Rallypoints
  • if the entity has been destroyed (because then it won't be rendered anymore)

Notice as of rP18848/rP19217, the RallyPoint component deletes all rallypoints upon capturing.
However the RallyPointRenderer currently pretends to care about capturing while failing to do so properly as it misses an ConstructAllOverlayLines.

Therefore fix the RallyPointRenderer capture support, so that modders and special maps can still persist rallypoints upon capturing change
without changing C++ code (even if it is currently an occasional, small waste of performance).

Test Plan

Compile with the patch and check for regressions in the following situations:

Test 1:

  1. Remove the OnOwnershipChanged function from RallyPoint.js
  2. Start a game with revealed map
  3. Set some rallypoints with shift-click
  4. Use alt+d to switch to the enemy perspective.
  5. Select that building and click on control-all-units to reveal the enemy rallypoints.
  6. Use the "wololo" cheat.

Notice that the rallypoint colors have the new player color and new civ markers.
Notice that the selection ring has the correct color.

Test 2:

  1. Open a scenario map in Atlas.
  2. Select an entity without owner, i.e. a decorative and notice a white selection ring.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Aug 9 2017, 1:39 PM
Vulcan added a subscriber: Vulcan.Aug 9 2017, 4:51 PM

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/1827/ for more details.

Vulcan added a comment.Aug 9 2017, 4:52 PM
Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/371/ for more details.

Stan accepted this revision.Aug 9 2017, 10:50 PM

Tested the two scenarios and couldn't find any regression.
The code looks nice, I miss the point to the minimap.cpp changes but I guess that's just for clarity sake.

I can also confirm it fixes a bug when updating the color while the simulation runs in atlas, where previously you'd have had to reset the flag to see the color change when changing the color in the left menu.

This revision is now accepted and ready to land.Aug 9 2017, 10:50 PM
In D776#30789, @Stan wrote:

Tested the two scenarios and couldn't find any regression.
The code looks nice

I miss the point to the minimap.cpp changes but I guess that's just for clarity sake.

Indeed it's just for clarity.
INVALID_PLAYER is -1, using the former means

  • we have it consistent throughout the code
  • can change the constant without changing all the other code if we wanted to
  • the former transports the meaning of the magic number

I can also confirm it fixes a bug when updating the color while the simulation runs in atlas, where previously you'd have had to reset the flag to see the color change when changing the color in the left menu.

Can't reproduce, but not relevant.

Thanks for the testing!

source/simulation2/components/CCmpRallyPointRenderer.cpp
235 ↗(On Diff #3061)

(Confirmed with a test that this is actually triggered)

source/simulation2/components/CCmpSelectable.cpp
369 ↗(On Diff #3061)

Tested with hero that starts with a selection ring before selecting.

It's actually the UpdatePlayerColor(); call in RenderSubmit and I'm not sure why that call is there.
Luckily that is out of scope of this proposal.

The "upon first selection" part is wrong, it should be upon first rendering.

This revision was automatically updated to reflect the committed changes.