Paths

Table of Contentst

Differential D289

Display Relics in the hero panel
ClosedPublic
Actions

Authored by elexis on Apr 4 2017, 4:19 AM.

Details

Reviewers

Sandarac

Commits

rP19398: Display relics in the hero panel.

Summary

Heroes are shown with separate icon shortcuts in the top panel.
Since #3000, we have space for an arbitrary amount of buttons there (currently limited to 10).
In the relic game mode, those entities can be very hard to find sometimes (as they arguably should, to keep it interesting).
Once owned, they should be found quickly and it seems this panel would be the perfect place (in particular a better place than the custom shortcuts).
Furthermore this would also make it much better for observers / replay viewers to find the relics after the game has started.

The task of finding relics of enemies can consume minutes, even with a revealed map.
This should be rather addressed with an expensive bribe feature in my opinion.

The proposed patch implements the feature, but the panel must be renamed, so that we can display arbitrary entities there without lying in the code.

The one requesting changes should state the final name (as the patch will be 95% renaming).
Proposals:
CrucialEntities? (Might be mistaken for something simulation relevant).
HighlightedEntities? (Wrong, since we have other highlighting of entities in the session code already)
PanelEntities?

Test Plan

Do a case sensitive search for "heroes" in the JS code and see that that Player component function is solely used for this panel feature.
'Make sure that it works' is redundant I guess.

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

elexis created this revision.Apr 4 2017, 4:19 AM

This will be nice to have - I guess maybe PanelEntities would work.

This revision now requires changes to proceed.Apr 4 2017, 4:33 AM

elexis edited the summary of this revision. (Show Details)Apr 4 2017, 5:08 AM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/690/ for more details.

Harbormaster completed remote builds in B1006: Diff 1092.Apr 4 2017, 5:11 AM

can the hero always displayed first in the row?

looks unfamiliar to me

Implements the rename to panelEntities. Notice we now have a panelEntitiesPanel.
Implements the sorting correctly suggested by ffffffff.
Adds wonder to that panel since it ought to be the most important structure and since it's the most important thing in Wonder victory mode.

Should we add code to only display the wonder in wonder victory gamemode?
If so, should we ask the EndgameManager in the player component whether the gamemode is "wonder" and then add the "Wonder" class (hardcoded)?
Or should add that Wonder class to a new property in the gameTypeSettings of the EndgameManager and expose that to the sim components?
Or just remove the wonder from that list entirely?

elexis edited the summary of this revision. (Show Details)Apr 5 2017, 5:07 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/700/ for more details.

Harbormaster completed remote builds in B1019: Diff 1112.Apr 5 2017, 6:03 PM

Lionkanzen added a subscriber: Lionkanzen.Apr 5 2017, 6:09 PM

bb added a subscriber: bb.Apr 6 2017, 1:50 PM

bb added inline comments.

binaries/data/mods/public/gui/common/tooltips.js
84 ↗	(On Diff #1112)	s/template/entState Wondering whether this function should/could be used in the copies in selection_details.js
91 ↗	(On Diff #1112)	(leads to ugly 3300/3301 things but we already have that => out of scope)
binaries/data/mods/public/gui/session/session.js
160 ↗	(On Diff #1112)	Order in which ...
binaries/data/mods/public/gui/session/session_objects/panel_entities.xml
6 ↗	(On Diff #1112)	a space too many
7 ↗	(On Diff #1112)	When we add a gamesetup option for relics, it would most likely also be set to 12 (number of differerent catafalques). So we can have 12 + hero + wonder == 14 units, doesn't seem to fit into 12 objects. But more then 12 won't fit on min resolution (colliding with civ button). Perhaps sometime we need 2 bars for this, but ok for now.

Remove Wonder since I've found no good argument for actually having them and in this case the hammer-nail argument might apply.
Unifty current health tooltips as proposed by bb and fix the style issues identified.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/712/ for more details.

Harbormaster completed remote builds in B1043: Diff 1146.Apr 8 2017, 8:00 AM

If we add the wonder, then only in that gamemode. Trigger scripts should be able to extend that list, so that we can use it also for the treasure seeker woman on survival.

binaries/data/mods/public/gui/common/tooltips.js
84 ↗	(On Diff #1112)	But solely because it's the only one that never gets fed a template. All tooltip that sometimes get entState and othertimes a template should remain consistent.
91 ↗	(On Diff #1112)	I think it was about preventing units having 0HP but running around alive. I think I've been so annoyed by that 3301 that it's time for a round while at it. Thanks for reminding me!
binaries/data/mods/public/gui/session/session_objects/panel_entities.xml
6 ↗	(On Diff #1112)	Thanks
7 ↗	(On Diff #1112)	(Technically there are also maps with more than 1 hero and in campaigns there might be those with special characters.) 12 already colliding, but not an issue to me. Players can still look for the other pixels if they need the tech tree. Buffing to 14. Test using "iwanttopwnthem 20"

The diff works as expected. I'm not so sure about having panelEntityClasses in player.js though (as well as g_PanelEntities) - it seems a bit out of place.

binaries/data/mods/public/gui/session/selection_details.js
54 ↗	(On Diff #1146)	Not really sure if there needs to be a newline here.

This revision is now accepted and ready to land.Apr 9 2017, 7:49 AM

If we want a defined order, we will need one of both arrays. We need it in the GUI because the entities can be added at different times (at least its more straight forward to not pull the GUI order from the sim).

I'm not satisfied with the Player.js variable either.
It's not inside the prototype because it doesn't need to be serialized (saving some space when rejoining / savegames).
It's not the first time we do this, see binaries/data/mods/public/simulation/components$ grep -R "^var".

It should be removed entirely once we support adding those classes from trigger scripts (wonder victory, survival treasure seeker) and templates (hero).

Thanks for the testing!

ffs!!
There was a bug.
When hovering the icon, the overlay image was replaced onTick and kind of flashing.
Took more than an hour rerereading all the renamed vars to find the place where it was wrong.

binaries/data/mods/public/gui/session/session.js
930 ↗	(On Diff #1146)	The new `panelEntIndex` function could be defined after the loop
931 ↗	(On Diff #1146)	This one was wrong because it switched panelEnt and ent! Argh!