Decouple PanelEntities code from session and use object orientation
ClosedPublic
Actions

Authored by elexis on Oct 22 2019, 6:27 PM.

Details

Reviewers: None
Commits: rP23089: Decouple panel entities code from session code and use class notation, refs…
Trac Tickets: #5387
#3000

Summary

This patch decouples the panel entity GUI code from the global session code and uses the class syntax to achieve that, refs #5387.

The code logic is changed a bit (while the behavior visible to the player is the same) in order to perform less useless work.

Buttons are created at one point in time and only updated with simstate relevant code.
The buttons are only repositioned if a button was inserted or deleted.
Each buttonhandler will use the same button throughout its lifetime rather than mapping dynamically.
sprite, onPress and onDoublePress are ressigned upon addition / removal of the button and perform the lookup on the entityID directly rather than iterating / lookup to find the correct entity
updateGUIStatusBar was reduced to 3 lines (changing one property of the size property) due to lack of other use cases and GUI resizing being simple enough to make the helper an unneeded indirection and odditiy instead of a timesaver
Engine.GetGUIObjectByName only upon construction of a button / manager to avoid repeated useless work
Health bar resizing only if health changed

These changes may help to make it easier to animate the button position instead of instant changes, as the TODO from rP18361 suggests.

The performance analsis shows that this is only slightly faster, < 5%, because there are two heavy performance blockers here.
One is the GetEntityState and the other is the tooltip functions (which also call GetEntityState).

Those two consume 300 microseconds per button on my machine whereas the rest of the new update function consumes something like 30 microseconds.
So the result is that we get yet another hint that GetEntityState interface needs to become smarter.

The tooltip functions equally. Perhaps one can make it so that the tooltip work is not performed anymore unless the tooltip is shown.
It would be a C++ patch, so thats only a prospect.

Everything besides those two evil calls should have been reduced to noisefloor, at least it's all 1, 5, or otherwise < 30 microseconds,
see the code comments in the first version of the patch uploaded.

Attached a measurement showing something like 13 relics in observerview selected, and the same replay from player 1 perspective who gained and lost relics and a hero.
(Notice the first graphics has a horizontal offset)

chart.ods55 KBDownload

replay used to measure:

commands.txt45 KBDownload

replay used to verify behavior:

commands.txt21 KBDownload

Notice that it now uses 15 microseconds where it had 80 microseconds before in case there are no panel entities, yay.

Test Plan

Correctness of the patch is ensured by testing that ingame everything works as it should, i.e.

having relics and training a hero results in the hero being inserted first (defined order)
gaining more relics than slots are available does not result in an error (fixed by D1397/rP23074)
All items are shown to the observer view, but if a player is selected, only the entities of that player
Add warnings to insert and delete functions and see that they are only called if a panel entity changed ownership, thus ensuring to minimize performance impact. Thereby also make sure that there is no stupid insert+delete again bug every call if having reached the button limit.
Measure each individual JS statement to see the performance weighting and see that this is optimal if one does not want to touch GetEntityState or tooltip code.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 9858
Build 16616: Vulcan Build	Jenkins
Build 16615: Vulcan Build (Windows)	Jenkins
Build 16614: arc lint + arc unit

Event Timeline

elexis created this revision.Oct 22 2019, 6:27 PM

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

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

Harbormaster failed remote builds in B9858: Diff 10193!Oct 22 2019, 6:28 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/481/display/redirect

elexis edited the summary of this revision. (Show Details)Oct 22 2019, 6:37 PM

The tooltip.map().filter().join() pattern is one that does some needless array construction.
However that is so little work that it makes no performance difference over a reduce call that creates the string directly:

		// 150 microseconds
		let foo = Engine.GetMicroseconds();
		if (false)
		this.panelEntButton.tooltip =
			this.nameTooltip +
			this.Tooltips.map(tooltip => tooltip(entityState)).filter(tip => tip).join("\n");
		else
		{
			let tooltip = this.nameTooltip;
			for (let tooltipHandler of this.Tooltips)
			{
				let ttip = tooltipHandler(entityState);
				if (ttip)
					tooltip = "\n" + ttip;
			}
			this.panelEntButton.tooltip = tooltip;
		}
		warn(Engine.GetMicroseconds() - foo);

Here again the code with the comments that show the performance imbalance:

	update(i, reposition)
	{
		// TODO: Instead of instant position changes, animate button movement.
		if (reposition)
			setPanelObjectPosition(this.panelEntButton, i, Infinity);

		// 150 microseconds
		let entityState = GetEntityState(this.entityID);

		// 20 microseconds
		if (this.hitpoints != entityState.hitpoints)
		{
			let size = this.panelEntityHealthBar.size;
			size.rright = 100 * entityState.hitpoints / entityState.maxHitpoints;
			this.panelEntityHealthBar.size = size;
		}

		// TODO: only  build tooltip if the object is hovered?
		// 150 microseconds
		this.panelEntButton.tooltip =
			this.nameTooltip +
			this.Tooltips.map(tooltip => tooltip(entityState)).filter(tip => tip).join("\n");

		// 1 microsecond
		if (this.hitpoints > entityState.hitpoints)
			startColorFade(this.overlayName, 100, 0, colorFade_attackUnit, true, smoothColorFadeRestart_attackUnit);
		this.hitpoints = entityState.hitpoints;
	}

nani awarded a token.Oct 22 2019, 6:49 PM

elexis added inline comments.Oct 22 2019, 7:41 PM

binaries/data/mods/public/gui/session/PanelEntity.js
70	onPress supports the `selection.add` hotkey, onDoublePress does not, so adding it to the latter, including ResearchProgress. After implementation I notice that every doublePress two press events are triggered, so screw that.