[PATCH] Inconsistent display of researchable techs when several structures are selected

[mimo]

When selecting several structures, the production queue displayed is the sum of all production queues, but not for researchable techs where only those of the first selected are displayed.

[sanderd17]

Please follow our Coding_Conventions, particular the braces. And perhaps using the Array.concat function is easier than the Array.push after all.

The rest looks fine (I didn't test it yet though), but if you want more direct feedback, it can be a good idea to join us on IRC: ​https://kiwiirc.com/client/irc.quakenet.org#0ad-dev

(don't forget to add the "review" tag again once you improved the patch).

And thanks for the patch ofc ;)

[elexis]

(Someone could also try to replace that magic number in the surrounding code)

[elexis]

In 18194:

Show researchable techs of all selected entities, not only the first one. Fixes #3288.

[elexis]

It was also needed to filter duplicate elements, so .concat has not been used.

Also the previous revision didn't show any techs if there are more than 8 but less than 17 trainable units shown. Now the first two rows can show 16 trainable units and below that 8 researchable techs.

[mimo]

There is a problem with r18194: if you select several structures and start one of the research from any structures which is not the first of the selection, the research will be affected to the first one even if it cannot research it.

Also some points which could be improved:

• allowing 16 trainables is not a good idea when we have tech pairs.
• currently, in the display of techs, when one is researched (and has no superseding ones), we leave a hole. I don't know if there was a good reason for doing so, but this should be removed (at least when multiselection) to be able to see more techs.
• we already have in sessions.js two globals g_allBuildableEntities and g_allTrainableEntities which are only updated when the selection changes. We could use a g_allResearchableTechs which would follow the same path, thus avoiding to recompute this list every turn.

[sanderd17]

• currently, in the display of techs, when one is researched (and has no superseding ones), we leave > a hole. I don't know if there was a good reason for doing so, but this should be removed (at least when multiselection) to be able to see more techs.

I think it makes it more predictable where to click. Say you want to research tech 1 and 2 from the building, and when clicking tech 1, the list shifts. This can be considered a bit annoying.

And yes, the 8 was there because techs need two rows (even if there's only one tech pair), so there can be only 8 other icons.

[elexis]

In 18197:

Revert r18194 as it's broken in several ways and revealed a simulation bug. Refs #3288.

[elexis]

This is by far more broken than I thought :/

• Researching tech at the wrong building: When selecting multiple buildings, the tech is queued at the first building originally selected (not the first one displayed in the selection panel). This way one can research a tech at the wrong building. This should be prohibited in both the simulation and gui.

• Number of used rows: It is true that the tech's cant show up if there is a tech pair and 16 trainable units. When selecting multiple buildings, it seems indeed better to show only trainable units rather than techs. If the player really wants to research a tech, he should just select the appropriate building, while selecting multiple buildings to train units is more useful.

• Shifting: With that commit, if you select f.e. only a tower and queue multiple techs, the space between the place of the last tech will shift. However it doesn't shift if you research only one tech.

• Meh to the cache: The panels are indeed updated once per turn (i.e. 5 times per second in SP). Thus, a cache would eliminate calls to a loop over the selected entities that loops over its researchable techs. However I'm not completely convinced that the benefit of the cache outweighs the cost. The cost are increasing the memory footprint, adding a global which doesn't contain any new information, thus violates the principle of "single source of truth" and "dont repeat yourself", adding code which has to be read and is only as useful as the performance benefit.

The performance benefit on my machine seems to be 1 microsecond per selected entity (measured with getTime and by looping 1000 times over the mentioned code).

Similar to this there are some caches in session.js to avoid getting the same information from a GUIInterface call which I don't like either. If we decide to use this kind of optimization, we could use it in many places, resulting in dozens if not hundreds of variables storing duplicate information.

If we want to update the panel only when the selection changes, we should avoid calling those functions in the first place. Also it may or may not be wrong to cache this information as the number of techs might change independently from the selection.

• Usability feature: When researching techs or training units at multiple buildings, it could queue them in a building that is not busy instead of just queueing it in the first N buildings in the selection. leper seems to be opposed to that one as we might endup with a messy file like UnitAI if we make things too intelligent and thus add too many checks. Though not sure if this would add really so much complexity in this case.

[elexis]

In 18773:

Correctly handle commands and production for multiple units and mixed selections. Patch by Imarok, reviewed by Itms, fixes #3823. Fixes #3492, #3288, #3668.