[PATCH] GUI: Hero icon area in upper left limited to one hero at a time

[brian]

The hero icon area in the upper left of the screen should provide an icon for each hero the player has. In normal gameplay only a single hero can be trained at a time. However, since scenarios may want to allow multiple heroes, we should include all the heroes that a player has in the hero area (with some reasonable limit, like 10).

An example scenario with multiple heroes is the Gaul Sandbox Demo map.

[leper]

Some care should be taken when implementing this as multiple hero icons will overlap with unit group icons (Ctrl-1 to 0 to create).

[historic_bruno]

I don't think UI space should be dedicated to something that should never happen. Maybe they could cycle in the (rare/impossible) case of multiple heroes?

[Stephen Imhoff]

Modified the hero display to place things in a horizontal panel, with a limit of 10 elements. Hero 11+ aren't displayed, but will show up if previous heroes are removed. This patch also fixes an issue where, with multiple heroes, the "being attacked" overlay was played on the next hero to occupy the same slot.

This patch should function in the case that the player has somehow acquired additional heroes, and tag them on the end, but this hasn't been tested yet.

[Stephen Imhoff]

Updated patch from initial round of code review.

Patch has been successfully tested with a map that spawns more player heroes; new heroes are tacked on the end, and slots are reused.

[Stan]

You might want to get rid of

//[START] of hero display helper functions 

as this type of comments isn't used anywhere in the file, and it doesn't help to understand the code better IMO.

Don't know if there isn't a cleaner way to declare that button in the code. I don't have any example on how it's done elsewhere.

[Stephen Imhoff]

...and comments removed.

[elexis]

Style remarks:

• Stacking functions inside each other is not that convincing. You can make one statement out of sortHeroes and we don't really need a function for setupHeroButton. The function createTooltip seems okay, but it is probably better make it a global, so it isn't redefined on every call.
• savedHero, oldButton unneeded variables
• increments: i++ should be ++i (convention)
• sortHeroes:
  • any reason for that elements argument and not accessing that global directly?
• setupHeroButton do we need a function for that at all?
• buttonSlot =... unneeded parenthesis
• Move the template init a bit further down, so it's not defined before being used
• for( -> for (
• L875-879 the comments don't really add anything
• merge lines 803/804 and respectively 809/810. is that slice unneeded?
• healthLabel can be inlined

Didn't test yet and maybe some things can be further simplified

[elexis]

Likely issues:

• "previous": g_PreviousHeroes[ent] -> if you save a copy of the previous object to the new object with every call, that thing will grow with every call, sounds unhealthy.
• Does that patch actually work for observermode when changing the perspective from one player to another? Shouldn't g_PreviousHeroes be cleared then?

The code is complicated:

• I don't like relying on usedSlots, it would be nicer if we could just sort the heroes from left to right by entity id and keep the logic more simple.
• If there are no heroes, all unitHeroImage buttons ought to be hidden. The code could be written to not affect unitHeroPanel object in the first place.
• If I understood correctly, the last loop just runs from mapped.length to g_HeroPanelCount?

Minor codestyle issues:

• Please don't add whitespace to emptylines or trailing whitespace in JS, nor XML.
• Close the object on a separate line.
• Too many lines have more than 100 characters, try to stay below 80 if possible by using newlines elegantly.
• onpress -> onPress
• Add some more newlines.
• hero.previous !== undefined -> hero.previous
• One of your comments doesn't start with a capital letter while all others do. Same goes for dots at the end of sentences.
• for( -> for (
• Though the coding convention say new Array should be avoided, it is a nicer way to produce the array usedSlots. (Does that code actually need that false-array? An empty array looks like it could work with some adjustments)

[elexis]

Removed trailing whitespace, added linebreaks, changed indentation and the like.

[Stephen Imhoff]

Thanks about whitespace (I need to look for an add-in for this, because I hate it in those situations too) and indentation (still working on figuring out where you guys want things).

Updated and posted. Apparently adding an indexed item to an array adds slots for everything "below" it too, so had to switch some logic around to avoid extremely large arrays.

Likely issues:

• "previous" isn't persisted back into g_previousHeroes (only numeric values are), so the objects can't grow. g_previousHeroes is also re-assigned each call, so will never be more than 10 elements (or however big the panel element is).
• Works (tested), and the previous heroes will get reset on switch; since they aren't in the previous heroes, resets everything.

The code is complicated:

• We need usedSlots (for efficiency). The heroes mostly are sorted by id (that's the order they get when first grabbed), the problem comes when they die. The initial implementation just assigned them hero1 -> slot1, but this caused the damage animation (blinking overlay) to be applied to a still-living hero when they disappeared, or to switch when their own overlay was if a preceding hero died (because the hero got moved up a slot). So I broke the link between slot id and order; this also avoids having to re-grab portraits and rebuild interaction events.
• &shrug& I was just going off of what the existing behavior did, changed. It requires looping through all the buttons (or at least the ones used by the previous heroes), so added a check to only hide them if they were in use.
• I think you missed something: the main loop (where slots are assigned and pictures displayed), the loop is whichever is shorter (since there may not be 10 heroes).

Minor codestyle issues:

• Completely agree, thanks again.
• ...I must have misunderstood an earlier comment elsewhere, I think.
• Okay.
• Okay. (But that was pre-existing code, and JS is case sensitive, so had no reason to anticipate changing case of external call would work)
• Thanks!
• Coding style docs state to explicitly check for undefined, though (I originally had it with the implicit check), which is why I switched (but now switched back the single remaining one). hero.previous is undefined if it wasn't included in the object, right? Or am I missing something about JS?
• Oops. Thanks again.
• .... I know I checked and fixed at least one of those.
• Can't use an empty array of Booleans, since what we're actually working with is the index (of the array). An empty array that we added the slot index to would require sorting after the last previous hero (and every new hero thereafter), and the required conditions for the find aren't pretty. You could start with a filled array of slot indexes and remove them, but that would require traversing the array each time and shifting elements. Much simpler, easier, and faster to just maintain bool flags (already displayed heroes don't search the array again).

[Stephen Imhoff]

remove separate free-slot array

[Stephen Imhoff]

elexis => You were right, usedSlots could be removed. I also managed to pull out the button creation. I'm not really seeing anything I could be doing to make things simpler, though. Do you have any specific pointers? More things could be cached, I guess, but that wouldn't make the code simpler, or at least I don't think so (you end up with lots of conditions for special cases, extra loops).

I looked into caching multiple players' heroes, but that gets convoluted pretty quickly. I can do it, that's not the problem, it just depends on how much you want to go on in the code.

[elexis]

Looks a lot cleaner already, but... :)

Major:

• Do we really need the .hidden and return part of the first check in updateHeroes? Can't we just set previous heroes to empty and let the rest (hiding it) happen below? This way you'd have exactly one place in the code that sets .hidden
• Still not happy with having this mapping in the first place:

  // The "slot" is which button, but it's the identity (name) of the button, not its position.
  // Heroes stay in the same slot, but the position of the button may change. 
  

  nor happy with new Array(g_HeroPanelCount).fill(undefined);. (As far as I understand your code after a brief look), you sort the simdata playerState.heroes according to display order and then create an array heroes which maps from the display order to the simdata. This can't be right. It should be sufficient to map directly from simData to GUI order and set the GUI attributes directly (only once, after all data processing is done, like in the Input-Process-Output Model).

Is the need to stopColorFade when a hero died the only reason to do that mapping? Shouldn't that be fixed in ColorFades.js (possibly by using the entityID instead of the colorFade id?).

Minor:

• Probably better to call g_HeroPanelCount g_MaxHeroPanelCount or g_HeroMaxPanelCount or similar
• length == 0 and length > 0 should just be !length and length
• move the init code to initGUIHeroes(i), this way init will stay shorter and maybe it helps with eliminating the closure too. The comment won't be needed anymore either.
• Unneeded comment:

  // This does a linear search through the array, 
  // but the multiple-hero case is extremely rare. 
  

  It is true that having multiple heroes is only rarely the case for now. However since it's the purpose of the ticket to handle the multiple-heroes case properly, that can't be an argument. As the linearity is self-evident too, it can be removed

[Stephen Imhoff]

Still not happy with having this mapping in the first place:

// The "slot" is which button, but it's the identity (name) of the button, not its position. // Heroes stay in the same slot, but the position of the button may change.

nor happy with new Array(g_HeroPanelCount).fill(undefined);. (As far as I understand your code after a brief look), you sort the simdata playerState.heroes according to display order and then create an array heroes which maps from the display order to the simdata. This can't be right. It should be sufficient to map directly from simData to GUI order and set the GUI attributes directly (only once, after all data processing is done, like in the Input-Process-Output Model). Is the need to stopColorFade when a hero died the only reason to do that mapping? Shouldn't that be fixed in ColorFades.js (possibly by using the entityID instead of the colorFade id?).

The reason I'm not mapping sim order to display order is that gaining a hero would potentially be disruptive to a player, if the hero is "earlier" (removals are disruptive, but are signaled beforehand). It's a separate issue from breaking the slot/display ordering link. That was broken to deal with colorFade issues (both stopping and keeping them going, in the case of multiple heroes and an earlier one dies first). In theory the fades could be moved between buttons (requires new method, I haven't tested it yet). Fades only have one name, and are attached directly to buttons anyways, so I'm not seeing any way to "fix" the situation except switching the fades or breaking the link like I did. Note that switching out the contents of the button would make it more difficult to do things like animate the buttons (say, slide over in the case of an earlier hero death), since you'd have a moving target.

I have a fix for all the other stuff that you mentioned, but the one I just uploaded is actually different, in that it split the update and display cycle (which would make it easier to update all heroes, even ones for other players). Is that more what you meant for IPO? I'm reasonably certain that doing switching with static buttons isn't going to gain much in terms of code simplicity.

[Stephen Imhoff]

split update/display cycle.

[elexis]

In 18361:

Implement buttons for up to 10 heroes. Patch by Clockwork-Muse, fixes #3000.

[elexis]

Thanks for the brave rewrites! :)

[elexis]

Potential use of a cache: The template could be copied to g_Heroes to avoid grabbing it every tick. However it is cached already in g_TemplateData and for any stupid unexpected the reason the template of the entitystate might update, so not doing that.

Here the things I changed:

• I think you want some... (instead of find)
• Used for (let slot in ...children) instead of a .length check
• Lines should end with an operator or semicolon (+ "\n" + ), that brace should be closed in the same level of indentation
• button instead of Engine.GetGUIObjectByName("unitHeroButton[" + slot + "]")
• Extended the comment of g_Heroes
• two spaces: g_Heroes = g_Heroes...
• Removed 1 tab indentation from buttons.forEach... and colorFade_attackUnit...
• Removed // Setup tooltip
• Few newlines

[Stephen Imhoff]

Argh, yes, that should have been some (was wanting includes again). I didn't realize for (let slot in ...children) would work, that's much cleaner, yes.

[elexis]

In 18365:

Fix whitespace, refs #3000.

[elexis]

In 19398:

Display relics in the hero panel.
Rename the buttonset to panelEntities and unify related health tooltips.

Differential Revision: ​https://code.wildfiregames.com/D289
Reviewed By: Sandarac
Refs #3000

[bb]

In 23074:

Fix PanelEntButton undefined, by not trying to fill gui elements that don't exist

Comments by: elexis
Differential Revision: ​https://code.wildfiregames.com/D1397
fixes: #5504
refs: #3000

[elexis]

In 23089:

Decouple panel entities code from session code and use class notation, refs #5387, #3000, #1902, #1802, rP18361.

Change the logic to only insert/delete buttonhandlers on ownershipchange and update only the entitystate dependent part on simulation update.

Differential Revision: ​https://code.wildfiregames.com/D2387