Page MenuHomeWildfire Games

Remove the serialization of colors in Minimap
ClosedPublic

Authored by temple on Dec 7 2017, 6:39 PM.

Details

Summary

Remove the serialization of colors in Minimap. It serializes them only when they're the player color, but we serialize the player color in the Player component anyway, so it seems unnecessary. None of the other components that use color serialize it. The only consequence is that on deserialization we don't have access to the colors anymore, so we have to set them to a dummy color and then set them to the real colors after everything's been deserialized. That just means we need to call a function to set the color on session init, easy enough.

Test Plan

Test that the minimap colors are correct when loading saved games and on multiplayer late joins.
Note that in Atlas after changing the player color when you 'Play' again the minimap unit colors will be correct. (Previously they stayed the original colors even after resetting.)
I used the generic UpdateDisplayedColors because the plan with D754 is to update the colors of other components too, and I'd like to use the same function.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4162
Build 7320: Vulcan BuildJenkins
Build 7319: arc lint + arc unit

Event Timeline

temple created this revision.Dec 7 2017, 6:39 PM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Dec 7 2017, 6:39 PM
Vulcan added a subscriber: Vulcan.Dec 7 2017, 6:44 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
elexis updated the Trac tickets for this revision.Dec 7 2017, 8:52 PM
temple added inline comments.Dec 8 2017, 9:50 PM
source/simulation2/components/ICmpMinimap.cpp
1

2017

bb accepted this revision.Dec 23 2017, 6:54 PM
bb added a subscriber: bb.

Patch looks ok to me, tested rejoining a game (on same pc), and no OOS occurred, code looks same, but I am not authority on it, leaving it open a bit in case someone with more experience with these parts of the code wants to yell.

This revision is now accepted and ready to land.Dec 23 2017, 6:54 PM
elexis added a subscriber: elexis.Dec 23 2017, 6:58 PM

(only posting the inline comments from last time I looked at it)

binaries/data/mods/public/gui/session/session.js
279

(Ok to have it here because it is planned to call that more often than once)

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

(Minimap entities sounds a bit like a set of Entities specifically crafted for the Minimap independent from the units and structures)

863

UpdateDisplayedPlayerColors to give the player context.

877

(entities can be inlined)

source/simulation2/components/CCmpMinimap.cpp
212

Passing the owner as an argument means that the function has the freedom to be called with a wrong owner.
(Was the option to pass the color directly prefered?)

228

(I was looking for some const to replcae the 255 with a reference to u8 for fun (lib/types.h:typedef uint8_t u8) http://www.cplusplus.com/reference/cstdint/) but didn't succeed. anyway.)

temple updated this revision to Diff 4917.Dec 23 2017, 9:40 PM
temple marked 3 inline comments as done.

don't pass owner

source/simulation2/components/CCmpMinimap.cpp
212

I was never comfortable with that.
(I think for now we're going to let components figure out the color. If there's performance issues then we can consider changing it.)

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
bb added inline comments.Dec 24 2017, 3:52 PM
binaries/data/mods/public/simulation/components/GuiInterface.js
866–868

getGaiaAndNonGaiaEntities function as proposed in D503 would help perhaps

temple updated this revision to Diff 5022.Jan 1 2018, 6:32 PM
temple marked an inline comment as done.

use GetGaiaAndNonGaiaEntities

Vulcan added a comment.Jan 1 2018, 7:26 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
bb added a comment.Jan 7 2018, 12:08 AM

Performed a quick oos test on two instances on the same machine, didn't notice an oos (not that that is a proof though).

Serialization has been added in rP8449

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

inlinable?

866

nukable

source/simulation2/components/CCmpMinimap.cpp
1

Happy new year!!

191

why did we do that? serializing stuff?
comment added in rP10017, the floating point has been there since the creation of the file

See irc discussion on 2017-12-08

Vulcan added a comment.Jan 7 2018, 6:13 AM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
bb accepted this revision.Jan 7 2018, 9:00 PM

Atlas, saving, loading front doesn't fall,
captured trees/mines do not use the player color, also after saving and loading
oos rejoin test, still doesn't fail

source/simulation2/components/ICmpMinimap.cpp
1

gotta luv this

This revision was automatically updated to reflect the committed changes.