Page MenuHomeWildfire Games

[gui] add extra row to right selection panel
AbandonedPublic

Authored by Nescio on Jun 8 2020, 9:34 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Sep 13, 1:20 PM
Unknown Object (File)
Sun, Sep 1, 9:00 PM
Unknown Object (File)
Sun, Sep 1, 12:02 PM
Unknown Object (File)
Sat, Aug 31, 5:45 AM
F1476608: D2806.png
Jul 20 2020, 10:20 AM
F1476611: line_corner_top_middle.png
Jul 20 2020, 10:20 AM
F1428894: D2806.png
Jun 12 2020, 6:40 PM
F1428867: Capture d’écran 2020-06-12 à 18.05.34.png
Jun 12 2020, 6:06 PM
Subscribers
Restricted Owners Package
Restricted Owners Package

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

Currently the right selection panel accommodates three rows of eight icons, i.e. a maximum of 24. Some civs can already build 24 different structures, which means no additional structures can be enabled (e.g. D2801). This patch tweaks the gui to add a fourth row, so up to 32 can be displayed:

right_selection_panel.png (2×3 px, 7 MB)

D2806.png (422×2 px, 864 KB)

This adds more flexibility for future additions and mods, as well as for players who like to group different structures and units together (Ctrl+1 etc.).

While at it, indentation of the gui/session/selection_panels_right/*.xml files is also corrected.

[EDIT] Phabricator can have difficulty handling images, so here is the added png in case it doesn't work for you:

line_corner_top_middle.png (4×4 px, 132 B)

D2875 is an alternative approach, shrinking icons instead of increasing the panel size. See also https://wildfiregames.com/forum/index.php?/topic/28525-larger-panel-or-smaller-icons/&tab=comments#comment-401080

Test Plan

Check for completeness and correctness.

Event Timeline

Owners added a subscriber: Restricted Owners Package.Jun 8 2020, 9:34 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'state'.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 298| 298| 		if (!g_AvailableFormations.has(unitEntStates[0].player))
| 299| 299| 			g_AvailableFormations.set(unitEntStates[0].player, Engine.GuiInterfaceCall("GetAvailableFormations", unitEntStates[0].player));
| 300| 300| 
| 301|    |-		return g_AvailableFormations.get(unitEntStates[0].player).filter(formation => unitEntStates.some(state => !!state.identity &&  state.identity.formations.indexOf(formation) != -1));
|    | 301|+		return g_AvailableFormations.get(unitEntStates[0].player).filter(formation => unitEntStates.some(state => !!state.identity && state.identity.formations.indexOf(formation) != -1));
| 302| 302| 	},
| 303| 303| 	"setupButton": function(data)
| 304| 304| 	{
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 761| 761| 				addResearchToQueue(data.item.researchFacilityId, t);
| 762| 762| 			})(tech);
| 763| 763| 
| 764|    |-			button.onPressRight = (t => function () {
|    | 764|+			button.onPressRight = (t => function() {
| 765| 765| 				showTemplateDetails(
| 766| 766| 					t,
| 767| 767| 					GetTemplateData(data.unitEntStates.find(state => state.id == data.item.researchFacilityId).template).nativeCiv);

binaries/data/mods/public/gui/session/selection_panels.js
|  50| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  61| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 729| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2381/display/redirect

It is something that we obviously need at the current stage of development, 24 do not support anymore.

Well, I don't know much about the code, so I can't give a technical opinion here if is correct or not.

It seems to work well and maintain the exact proportions.

It doesn't quite look right when producing something, and further the border is cut weirdly.

Capture d’écran 2020-06-12 à 17.19.25.png (374×338 px, 211 KB)

I guess there's no real way to squeeze one more column though, which I'd prefer. The dominance of the middle panel looked quite nice imo.

I recall Freagarach has a diff for tabs somewhere.

Edit -> specifically

Capture d’écran 2020-06-12 à 18.05.34.png (274×272 px, 58 KB)
is not aligned properly and the sprite is doing something odd at the border

Owners added a subscriber: Restricted Owners Package.Jun 12 2020, 6:38 PM

It doesn't quite look right when producing something, and further the border is cut weirdly.

Well spotted, thanks for pointing that out! Those two things are now fixed:

D2806.png (322×452 px, 196 KB)

I guess there's no real way to squeeze one more column though, which I'd prefer. The dominance of the middle panel looked quite nice imo.

The alternative is shrinking the icon sizes instead.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2411/display/redirect

I recall Freagarach has a diff for tabs somewhere.

I don't think I managed to get further than a ticket (#5613) actually.

I have to say - I would much prefer to not merge this :/
I think it's somewhat uglier, and somewhat less convenient given the space it takes.
I also think people will want to fill that empty row with new buildings, and we'll end up with building creep.
I think we already have too many buildings :/ .

With regards to adding new buildings:

  • Do we really need 3+ different towers? It seems to me we could remove the "intermediate" tower.

Again, the alternative is keeping the panel as is and reducing the icon size instead. I could make a patch for that, allowing you to compare the two and decide which one is the least ugly. The current limit of 24 is increasingly problematic, though, not just for the public default, but also for mods.

Do we really need 3+ different towers? It seems to me we could remove the "intermediate" tower.

Personally I think those new city phase towers shouldn't have been enabled (rP23094, rP23159, rP23160, rP23179), they're a mess, hence D2495.

Again, the alternative is keeping the panel as is and reducing the icon size instead.

There is of course the 3rd alternative of doing panels like Freagarach suggests.
24 buildings for an RTS seems like it should be workable enough to me, at least for the public mod. I don't mind having the capability to do more. Ideally you could just change a constant somewhere and the code would self-adjust.

I could make a patch for that, allowing you to compare the two and decide which one is the least ugly.

I think scaling down might be more scalable, if you want to give it a go.

Personally I think those new city phase towers shouldn't have been enabled (rP23094, rP23159, rP23160, rP23179), they're a mess, hence D2495.

If we were to disable those, then we wouldn't be pressed to merge this, correct? I'll give that patch a look.

If we were to disable those, then we wouldn't be pressed to merge this, correct? I'll give that patch a look.

No, currently the civ with the most structures is kush, at 24, and they didn't receive any new towers.

Since the alternative (D2875) has been committed (rP24028), this one (D2806) is no longer necessary.