After the Barter cleanup of rP20598, we no longer need cmpPlayerManager or playerEnt.
Details
Details
- Reviewers
elexis - Commits
- rP20629: Slight cleanup of Commands.js.
Includes a few other simplifications as well.
Diff Detail
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
| binaries/data/mods/public/simulation/helpers/Commands.js | ||
|---|---|---|
| 7 ↗ | (On Diff #4681) | Do we use INVALID_PLAYER in about every ownershipchange event and not a single itme in JS? |
| 10 ↗ | (On Diff #4681) | I do recall someone liking that we save components getter calls here but also that this is a bad design pattern everywhere else. Maybe it's a good exception or this one should be removed too. |
| 445 ↗ | (On Diff #4681) | meh, should have noticed |
| 743 ↗ | (On Diff #4681) | (Good Change if its indeed the same playerID in any case) |
Comment Actions
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...
Comment Actions
Thanks
| binaries/data/mods/public/simulation/helpers/Commands.js | ||
|---|---|---|
| 7 ↗ | (On Diff #4681) | That < 0 is a bit unclean. If it's a common case that we get -1 then we should test against that. That an observer sends a simulation command should be as impossible as it can get. |
| 10 ↗ | (On Diff #4681) | Ok I'm not becoming friends with this data.cmpPlayer variable, but I fully agree with not deleting it in this diff. The more I appreciate that the data.cmpPlayerManager is removed. Either we do cache components everywhere where possible or nowhere IMO. Also I'm adding 2 newlines, so that in case we ever add a second property, we will only have to change one character on that line and so that all objects I ever added have one line per property (code symmetry). In fact we can add the controlAllUnits property here as well. |
| 445 ↗ | (On Diff #4681) | (so this optimization goes contrary to what I would wish, but I dont mind) |
| 700 ↗ | (On Diff #4681) | good |
| 743 ↗ | (On Diff #4681) | (In particular accessing component variables directly is discouraged) |
| binaries/data/mods/public/simulation/helpers/Commands.js | ||
|---|---|---|
| 7 ↗ | (On Diff #4681) | It's kinda weird in PlayerManager.GetPlayerByID, where it says: // All players at or below ID 0 get gaia-level data. (Observers for example) if (id <= 0) return this.playerEntities[0]; So it seemed like maybe there was some potential for observers to control gaia? That's why I played it safe and kept the player < 0 check. But I never actually tested if observers could do commands. |
| binaries/data/mods/public/simulation/helpers/Commands.js | ||
|---|---|---|
| 7 ↗ | (On Diff #4681) | Tested, seems they can't, so okay. |