Currently the code for the Command selection panel loops through every selected item, to check if it can perform a command.
With the patch it checks it for the whole selection at once.
Therefore performCommand now gets the whole selection as parameter and not just the first element, too.
Details
Details
- Reviewers
Itms bb - Commits
- rP19537: Make g_EntityCommands use all entities but not only the first
- Trac Tickets
- #4273
- Check that everything works as before
- As observer select a cc from a player with mutual allies and one from a player without mutual allies: the shared-dropsite button should exist independent of selection order.
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
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/105/ for more details.
Comment Actions
I like the patch but we should highlight the remaining uses of [0] with a comment, ideally we should have an idea of what we want to do with them eventually.
| binaries/data/mods/public/gui/session/input.js | ||
|---|---|---|
| 1503 ↗ | (On Diff #148) | Since we are more and more considering all entities all over the code, it would be nice to add comments when we still consider only the first one (explaining why it makes sense here, or adding a TODO with improvement ideas). |
| binaries/data/mods/public/gui/session/unit_actions.js | ||
| 1074 ↗ | (On Diff #148) | Isn't there a better way to remove dupes? Asking the fat arrow master @elexis ^^ |
| 1187 ↗ | (On Diff #148) | A very nice thing to do would be cycling between the rally points in this situation but this might be out of the scope of the patch. Add a comment/TODO at least. |
| 1299 ↗ | (On Diff #148) | Same comment as above, although here we might want to check that all selected entities belong to the same player, what do you think? |
| binaries/data/mods/public/gui/session/unit_actions.js | ||
|---|---|---|
| 1074 ↗ | (On Diff #148) | In what way could there be a better arrow function for removing dupes? |
| binaries/data/mods/public/gui/session/unit_actions.js | ||
|---|---|---|
| 1299 ↗ | (On Diff #148) | You discovered a bug ? |
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/280/ for more details.
Comment Actions
some hunk got outdated, so didn't test if everything works.
| binaries/data/mods/public/gui/session/unit_actions.js | ||
|---|---|---|
| 1042 ↗ | (On Diff #401) | check added looks correct |
| 1051 ↗ | (On Diff #401) | argument needed? |
| 1059 ↗ | (On Diff #401) | good, since better performance than .every => isUndel(entstate) |
| 1090 ↗ | (On Diff #401) | Better indeed than g_selection |
| 1134 ↗ | (On Diff #401) | Shouldn't we use an entState.filter(entState => !entState.unitAI || !entState.turretParent) since we check sorta twice for the turretParent here (few lines down)?. |
| 1139 ↗ | (On Diff #401) | does that 3rd check make sense? as the entity already has this as turrentParent. |
| 1188 ↗ | (On Diff #401) | Should be entState = entStates.filter(entState => entState.rallyPoint && entState.rallyPoint.position)[0] || entStates.filter(entState => entState.position)[0] . And then also check if entState exists. I don't think this can be merged with the focusTarget assignment, because of empty lists |
| 1301 ↗ | (On Diff #401) | Players aren't allowed to have entity's selected of different players right? So I guess checking for 1 ent is enough. |
Comment Actions
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!
http://jw:8080/job/phabricator/1000/ for more details.
Comment Actions
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!
http://jw:8080/job/phabricator/1003/ for more details.
| binaries/data/mods/public/gui/session/unit_actions.js | ||
|---|---|---|
| 1152 ↗ | (On Diff #1645) | Probably in another cleanup patch, this function (and his brothers) should be inlined here, using the entStates parameter... |
| 1311 ↗ | (On Diff #1645) | GetSimState().players can be moved out of the function and stored under a variable. |
| binaries/data/mods/public/gui/session/unit_actions.js | ||
|---|---|---|
| 1311 ↗ | (On Diff #1645) | won't it help performance a bit? or is the interpreter smart enough to store it self? |
| binaries/data/mods/public/gui/session/unit_actions.js | ||
|---|---|---|
| 1311 ↗ | (On Diff #1645) | It should, and if not it won't bring us that much performance improvement... |
| binaries/data/mods/public/gui/session/unit_actions.js | ||
|---|---|---|
| 1311 ↗ | (On Diff #1645) | That assuming => accepting |