That a follow up of D1223 with additionnal cleanup: remove all the null initializations which are not useful and in fact that further improves the performance by about 5% when 200 units are selected.
Details
Details
- Reviewers
elexis - Commits
- rP20965: Cleaning of GetEntityState part 2
Check are all variables used in the gui are correctly tested before use.
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
I'd appreciate making those nulls a thing of the past. Especially because it means we only have one place where a property is set rather than having to keep thing in sync.
I guess one should spend some attention that the patch is complete, all calls to the properties adapted.
| binaries/data/mods/public/simulation/components/GuiInterface.js | ||
|---|---|---|
| 245 ↗ | (On Diff #5347) | To avoid the || false in gui/ and incidentally remove the if-statement+indentation here, we could also do ret.identity = cmpIdentity && {
....
};But I'm not sure if we want to do that. Guess it removes the 5% performance benefit. |
Comment Actions
Well, I've confirmed all properties up to miraging, but not the rest.
Seems some properties are unused, so we should remove them unless we find a good reason to have them.
fogging for instance.
I then searched for entEtate.
In input.js there is if (position && entState.visibility != "hidden"), maybe that could throw some nasty undefined warning, but the code also smells like it assumes that it's always defined. So maybe that can be ignored.
Of the subset of the matches I looked at, almost all of them had a negated or explicit check, so it might not bug.
Accepting this in case you are only waiting for the green checkmark. Before the commit the entState lines should be checked however.
| binaries/data/mods/public/gui/session/unit_actions.js | ||
|---|---|---|
| 1418 ↗ | (On Diff #5347) | Those are exactly the things easy to miss when patching and reviewing. (Could also go for double negation, not sure if it improves) |
| binaries/data/mods/public/simulation/components/tests/test_GuiInterface.js | ||
| 573 ↗ | (On Diff #5347) | correct, doesnt need a check |
| 574 ↗ | (On Diff #5347) | correct, doesnt need a check |