Page MenuHomeWildfire Games

Display correct ranged attack overlay for structures
Changes PlannedPublic

Authored by Angen on Mar 23 2019, 1:03 PM.

Details

Reviewers
None
Trac Tickets
#5207
Summary

When you place structure on some hill, it gets bonus from terrain, but overlay stays at maximum value from template what creates confusion when you look at tower range given by overlay and range in which tower can actually fire.

Test Plan

Should be good, but maybe something can be improved.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7039
Build 11507: Vulcan BuildJenkins
Build 11506: arc lint + arc unit

Event Timeline

Angen created this revision.Mar 23 2019, 1:03 PM
Vulcan added a subscriber: Vulcan.Mar 23 2019, 1:42 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 495| 495| 
| 496| 496| 		let horizSpeed = +this.template[type].ProjectileSpeed;
| 497| 497| 		let gravity = +this.template[type].Gravity;
| 498|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 498|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 499| 499| 
| 500| 500| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 501| 501| 		if (!cmpPosition || !cmpPosition.IsInWorld())
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 544| 544| 		let launchPoint = selfPosition.clone();
| 545| 545| 		// TODO: remove this when all the ranged unit templates are updated with Projectile/Launchpoint
| 546| 546| 		launchPoint.y += 3;
| 547|    |-		
|    | 547|+
| 548| 548| 		let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
| 549| 549| 		if (cmpVisual)
| 550| 550| 		{
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 615| 615| 			});
| 616| 616| 	}
| 617| 617| 	else
| 618|    |-	{
|    | 618|+	
| 619| 619| 		// Melee attack - hurt the target immediately
| 620| 620| 		cmpDamage.CauseDamage({
| 621| 621| 			"strengths": this.GetAttackStrengths(type),
| 625| 625| 			"type": type,
| 626| 626| 			"attackerOwner": attackerOwner
| 627| 627| 		});
| 628|    |-	}
|    | 628|+	
| 629| 629| };
| 630| 630| 
| 631| 631| /**
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["max"] is better written in dot notation.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 679| 679| 
| 680| 680| 	let range = this.GetRange("Ranged");
| 681| 681| 	let rangeOverlays = [];
| 682|    |-	let bonus = range["max"];
|    | 682|+	let bonus = range.max;
| 683| 683| 	let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 684| 684| 	let cmpUnitAI = Engine.QueryInterface(this.entity, IID_UnitAI);
| 685| 685| 	let cmpRangeManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_RangeManager);
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 683| 683| 	let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 684| 684| 	let cmpUnitAI = Engine.QueryInterface(this.entity, IID_UnitAI);
| 685| 685| 	let cmpRangeManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_RangeManager);
| 686|    |-	if (cmpUnitAI && cmpPosition && cmpPosition.IsInWorld()) {
|    | 686|+	if (cmpUnitAI && cmpPosition && cmpPosition.IsInWorld()) 
| 687| 687| 		bonus = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, 0);
| 688|    |-	}
|    | 688|+	
| 689| 689| 	else if(cmpPosition && cmpPosition.IsInWorld())
| 690| 690| 	{	
| 691| 691| 		// For buildings, take the average elevation around it. So angle = 2*pi
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'if' condition.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 687| 687| 		bonus = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, 0);
| 688| 688| 	}
| 689| 689| 	else if(cmpPosition && cmpPosition.IsInWorld())
| 690|    |-	{	
|    | 690|+		
| 691| 691| 		// For buildings, take the average elevation around it. So angle = 2*pi
| 692| 692| 		bonus = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, 2*Math.PI);
| 693|    |-	}
|    | 693|+	
| 694| 694| 	range["max"] = bonus;
| 695| 695| 	for (let i in range)
| 696| 696| 		if ((i == "min" || i == "max") && range[i])
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 687| 687| 		bonus = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, 0);
| 688| 688| 	}
| 689| 689| 	else if(cmpPosition && cmpPosition.IsInWorld())
| 690|    |-	{	
|    | 690|+	{
| 691| 691| 		// For buildings, take the average elevation around it. So angle = 2*pi
| 692| 692| 		bonus = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, 2*Math.PI);
| 693| 693| 	}
|    | [NORMAL] ESLintBear (dot-notation):
|    | ["max"] is better written in dot notation.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 691| 691| 		// For buildings, take the average elevation around it. So angle = 2*pi
| 692| 692| 		bonus = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, 2*Math.PI);
| 693| 693| 	}
| 694|    |-	range["max"] = bonus;
|    | 694|+	range.max = bonus;
| 695| 695| 	for (let i in range)
| 696| 696| 		if ((i == "min" || i == "max") && range[i])
| 697| 697| 			rangeOverlays.push({

binaries/data/mods/public/simulation/components/Attack.js
| 485| ·»   let·cmpDamage·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Damage);
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/components/Attack.js
| 686| »   if·(cmpUnitAI·&&·cmpPosition·&&·cmpPosition.IsInWorld())·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/simulation/components/Attack.js
| 590| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template.Ranged.Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.

binaries/data/mods/public/simulation/components/Attack.js
| 682| »   let·bonus·=·range["max"];
|    | [NORMAL] JSHintBear:
|    | ['max'] is better written in dot notation.

binaries/data/mods/public/simulation/components/Attack.js
| 694| »   range["max"]·=·bonus;
|    | [NORMAL] JSHintBear:
|    | ['max'] is better written in dot notation.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1129/display/redirect

Stan added inline comments.Mar 23 2019, 3:13 PM
binaries/data/mods/public/simulation/components/Attack.js
682

Range.max ?

685

No check for range manager ?

687

Could be unified in one if(cmppos && isingword)
Var x = cmpUnitai ? 2 PI : 0;
bonus = cmpRangeManager.GetElevationAdaptedRange(cmpPosition.GetPosition(), cmpPosition.GetRotation(), range.max, range.elevationBonus, x);

694

Range.max ?
Could be merged in the if after the change I guess.

elexis added a subscriber: elexis.Mar 23 2019, 5:08 PM

Didn't we have a ticket for it? I recall receiving a report about that once.
I also seem to recall having looked into the code and having gained the impression that range bonus was already accounted for.
Actually I get it again looking into the code and wondering why the elevation code in GetRange doesn't already provide the information.
I guess ElevationBonus != GetElevationAdaptedRange, the former must just have been height of the building.
Good then.

Might have a performance impact. Some stress testing and FPS comparison with large selections might help uncover possible performance issues.

binaries/data/mods/public/simulation/components/Attack.js
687

and inline x

Angen added a comment.Mar 23 2019, 6:22 PM

Right, elevationBonus is number based on which is added range corresponding to how high is current terrain.
It is based on GuiInterface.js lines 410 - 426 which result is used to display (+x) in toolip for range attack.

Actually based from CCmpRangeManager elevationBonus tells how much higher is the current source of attack. (so for towers it is window i suppose)
Buildings attack units which enter their query, that is created by cmpRangeManager.CreateActiveParabolicQuery. And for checking if some unit is
in query, it uses in final result the same function as to compute elevation adaptive range.

I guess it could be remembered for buildings once they are build? Or compute it only once? As this is not going to change for them.

Angen updated this revision to Diff 7611.EditedMar 23 2019, 6:58 PM

added missing check for change in elevation bonus

I have got the same fps drop by selecting 21 buildings all with elevationbonus when displaying range overlay was on and of.
Actually it was the same fps drop which I got by selecting 15 units.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/RangeOverlayManager.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/RangeOverlayManager.js
|  81|  81| 	this.RegenerateRangeOverlays(false);
|  82|  82| };
|  83|  83| 
|  84|    |-/** 
|    |  84|+/**
|  85|  85|  * RangeOverlayManager component is deserialized before the TechnologyManager, so need to update the ranges here
|  86|  86|  */
|  87|  87| RangeOverlayManager.prototype.OnDeserialized = function(msg)
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 495| 495| 
| 496| 496| 		let horizSpeed = +this.template[type].ProjectileSpeed;
| 497| 497| 		let gravity = +this.template[type].Gravity;
| 498|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 498|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 499| 499| 
| 500| 500| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 501| 501| 		if (!cmpPosition || !cmpPosition.IsInWorld())
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 544| 544| 		let launchPoint = selfPosition.clone();
| 545| 545| 		// TODO: remove this when all the ranged unit templates are updated with Projectile/Launchpoint
| 546| 546| 		launchPoint.y += 3;
| 547|    |-		
|    | 547|+
| 548| 548| 		let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
| 549| 549| 		if (cmpVisual)
| 550| 550| 		{
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 615| 615| 			});
| 616| 616| 	}
| 617| 617| 	else
| 618|    |-	{
|    | 618|+	
| 619| 619| 		// Melee attack - hurt the target immediately
| 620| 620| 		cmpDamage.CauseDamage({
| 621| 621| 			"strengths": this.GetAttackStrengths(type),
| 625| 625| 			"type": type,
| 626| 626| 			"attackerOwner": attackerOwner
| 627| 627| 		});
| 628|    |-	}
|    | 628|+	
| 629| 629| };
| 630| 630| 
| 631| 631| /**
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 679| 679| 
| 680| 680| 	let range = this.GetRange("Ranged");
| 681| 681| 	let rangeOverlays = [];
| 682|    |-	
|    | 682|+
| 683| 683| 	if (range.elevationBonus && range.max) {
| 684| 684| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 685| 685| 		let cmpUnitAI = Engine.QueryInterface(this.entity, IID_UnitAI);

binaries/data/mods/public/simulation/components/Attack.js
| 485| ·»   let·cmpDamage·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Damage);
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/components/Attack.js
| 683| »   if·(range.elevationBonus·&&·range.max)·{
|    | [NORMAL] ESLintBear (brace-rules/brace-on-same-line):
|    | Opening curly brace appears on the same line as controlling statement.

binaries/data/mods/public/simulation/components/Attack.js
| 590| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template.Ranged.Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1130/display/redirect

Stan added inline comments.Mar 23 2019, 7:23 PM
binaries/data/mods/public/simulation/components/Attack.js
688

Spaces for operator. 2 * PI instead of 2*PI

With the new message subscription, one could patrol on a hill with a unit that has the range displayed. Then the thing would be updated very often.

I would have recommended Engine.GetMicroseconds() to measure the time that this call takes, but that's not available to simulation context, only GUI.
But one can still use Date.now() (milliseconds) + for (let t = 0; t < 10000; ++t) around the measured call.
I guess the computation is negligible if it only happens once upon selection change. But if the unit moves a lot, it might become relevant.
(We might eventually want to enable it for units that the player doesn't have so many of like catapults, and mods might watn to do whatever)

In D1800#73394, @Angen wrote:

Actually it was the same fps drop which I got by selecting 15 units.

That's because GUIInterface GetEntityState is slow. That is slow because it gets all kinds of information. Just like this patch, more and more information was added.
So it's good if we know whether we're adding an expensive or a cheap tech, even if the GetEntityState event is called upon selectionchange, or once per simulation turn.

Buildings attack units which enter their query, that is created by cmpRangeManager.CreateActiveParabolicQuery.

(Was wondering whether one could remove a bit of duplication, but doesn't look like it.)

I guess it could be remembered for buildings once they are build? Or compute it only once? As this is not going to change for them.

built.
Maybe a cache would be good. It's a tradeoff. Saves CPU cycles for memory, so both factors would have to be weighed. One number for 2000 buildings each, maybe 4KB? I guess it's negligible here if units aren't going to receive it. But simulation cache variables might be useful to fix the selection-FPS-dropping GetEntityState.
Even if the state changes (here for instance simulation-modifiable terrain), the cache variable could be updated once the relevant events occur.

We don't have a ticket for the elevation range display problem?

Otherwise patch looks good (in my webbrowser).

binaries/data/mods/public/simulation/components/Attack.js
683

\n{

683

Why is there a range.elevationBonus test in the condition? Doesn't the elevationadapted range come into play even if that value above is 0?

688

spaces around the * operator.
176 characters that line, how about:

	range.max = cmpRangeManager.GetElevationAdaptedRange(
		cmpPosition.GetPosition(),
		cmpPosition.GetRotation(),
		...

\n before the for

cmpUnitAI init only if the condition is true

Angen added a comment.EditedMar 23 2019, 10:15 PM

It is recomputed just if object changes owner, is created or some modification comes. Overlay for units is not updated white moving even when they get elevation bonus. Using let now = Date.now() and logging Date.now() - now at the end of GetElevationAdaptedRange returns 0 or 1 depending on range, bigger maximum range and elevationbonus longer it gets to compute. However with 1000 loop it freezes. But is it actually such problem if range overlay is not updated on movement ? We could restrict this computation just for buildings and make TODO to solve it for units.

Angen updated the Trac tickets for this revision.Mar 23 2019, 10:33 PM
Angen updated this revision to Diff 7617.Mar 25 2019, 6:05 PM

Restricting this update to buildings. For units this to work it needs to enable recomputing range overlay on unit motion what could become expensive.

In D1800#73403, @Angen wrote:

But is it actually such problem if range overlay is not updated on movement ?

Well it says "Display correct ranged overlay for maximum range".
If one wants to intentionally semi-support it, then one should mention it in a code comment, otherwise people think it's just something missing that can be added, rather than an intentional omission.

GetElevationAdaptedRange returns 0 or 1 depending on range, bigger maximum range and elevationbonus longer it gets to compute. However with 1000 loop it freezes.

Could have tried with 10, 50, or 100. But it sounds like it's slow enough to not update it each move, if it's up to 1ms for one unit.

); on the previous line

The patch is still missing something. I don't know what hint I could give to you without revealing it. Perhaps that mods can do everything that the Schema allows. Or perhaps that one of your assumptions was not correct. Perhaps that reveals it already, but it's something about the relation of positionchanges and UnitAI, and about the task"Display correct ranged overlay for maximum range".

Angen retitled this revision from Display correct ranged overlay for maximum range to Display correct ranged overlay for maximum range of structures.Mar 25 2019, 6:53 PM
Angen added a comment.Mar 25 2019, 7:33 PM

Looped 100 times and 200 times and did not got bigger than 5 ms if recall correctly. But when I tried to update this overlay for placement, it was 1 ms for that building so I think it would not be wise update ranged overlay for units if they move. Thats why given information would be incorrect for them and more misleading as it would depend on the position they were spawned.

If some mod combines UnitAI and Structure, it will not be computed. If some entity does not have unitAI, it will not move using commands or will ? ( every order to move goes through UnitAI ).

( I think Summary starts with : "When you place structure on some hill" not if you would enable attack ranged overlay for unit :) )

Angen retitled this revision from Display correct ranged overlay for maximum range of structures to Display correct ranged attack overlay for structures.Mar 25 2019, 7: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-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/RangeOverlayManager.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/RangeOverlayManager.js
|  81|  81| 	this.RegenerateRangeOverlays(false);
|  82|  82| };
|  83|  83| 
|  84|    |-/** 
|    |  84|+/**
|  85|  85|  * RangeOverlayManager component is deserialized before the TechnologyManager, so need to update the ranges here
|  86|  86|  */
|  87|  87| RangeOverlayManager.prototype.OnDeserialized = function(msg)
|    | [NORMAL] ESLintBear (spaced-comment):
|    | Expected space or tab after '//' in comment.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 495| 495| 
| 496| 496| 		let horizSpeed = +this.template[type].ProjectileSpeed;
| 497| 497| 		let gravity = +this.template[type].Gravity;
| 498|    |-		//horizSpeed /= 2; gravity /= 2; // slow it down for testing
|    | 498|+		// horizSpeed /= 2; gravity /= 2; // slow it down for testing
| 499| 499| 
| 500| 500| 		let cmpPosition = Engine.QueryInterface(this.entity, IID_Position);
| 501| 501| 		if (!cmpPosition || !cmpPosition.IsInWorld())
|    | [NORMAL] ESLintBear (no-trailing-spaces):
|    | Trailing spaces not allowed.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 544| 544| 		let launchPoint = selfPosition.clone();
| 545| 545| 		// TODO: remove this when all the ranged unit templates are updated with Projectile/Launchpoint
| 546| 546| 		launchPoint.y += 3;
| 547|    |-		
|    | 547|+
| 548| 548| 		let cmpVisual = Engine.QueryInterface(this.entity, IID_Visual);
| 549| 549| 		if (cmpVisual)
| 550| 550| 		{
|    | [NORMAL] ESLintBear (curly):
|    | Unnecessary { after 'else'.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/simulation/components/Attack.js
| 615| 615| 			});
| 616| 616| 	}
| 617| 617| 	else
| 618|    |-	{
|    | 618|+	
| 619| 619| 		// Melee attack - hurt the target immediately
| 620| 620| 		cmpDamage.CauseDamage({
| 621| 621| 			"strengths": this.GetAttackStrengths(type),
| 625| 625| 			"type": type,
| 626| 626| 			"attackerOwner": attackerOwner
| 627| 627| 		});
| 628|    |-	}
|    | 628|+	
| 629| 629| };
| 630| 630| 
| 631| 631| /**

binaries/data/mods/public/simulation/components/Attack.js
| 485| ·»   let·cmpDamage·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Damage);
|    | [NORMAL] ESLintBear (no-mixed-spaces-and-tabs):
|    | Mixed spaces and tabs.

binaries/data/mods/public/simulation/components/Attack.js
| 590| »   »   cmpTimer.SetTimeout(SYSTEM_ENTITY,·IID_Damage,·"MissileHit",·timeToTarget·*·1000·+·+this.template.Ranged.Delay,·data);
|    | [NORMAL] JSHintBear:
|    | Confusing plusses.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1134/display/redirect

Well, I meant that one should try to work with the definition of the component properties more than with what the implementation currently does.
Who says that UnitAI is the only component to initiate a position change? It could also be a trigger script relocating a non-unitAI entity.
Or if the terrain is changed, then the height, and thus the elevation bonus can change as well for an entity.
There could be new components for arbitrary reasons that trigger position changes, it could be some scripted AI, or someones experimental alternative unitAI, or a magic tree that is moved in a cutscene.

Looped 100 times and 200 times and did not got bigger than 5 ms if recall correctly
But when I tried to update this overlay for placement, it was 1 ms for that building so I think it would not be wise update ranged overlay for units if they move

So you measured the entire function, not only the RangeManager call? Since the RangeManager has the same tasks in both cases, so I expect it to consume the same time in both cases.
The building preview is updated often, probably once per mousemove, whereas the units are likely updated less often (once per simulation turn interpolation?), and the code could be written so that it updates the range at most twice per second, or at most once per turn, per unit.
If it's 5ms for 100 selected moving units, that may be noticeable, but possibly still tolerable.

If we cannot afford to update the thing upon position change, then we should at least add a code comment to the place where the position-change-subscription would be found, and explain why it's not there.
But if it's remotely tolerable performance loss, then one can still mention in the Schema that it is highly discouraged to use the range overlay for units due to performance, but that template creators and modders can use it if they know there won't be many entities having it, or there won't be many movements (triggerscripted structure or terrain move for instance). Won't stop people who never read the specs nor the code, but the ones who do read it and might want to use the positionchange subscription would benefit.
One could even make a boolean to trigger the positionchange subscription, but that seems like bloat.
Better find a way to Display correct ranged attack overlay (i.e. never display incorrect ranged attack overlay) without any performance limits.

In the worst case one could look at the rangemanager too. (It was one of the indiegogo campaign goals to do that actually)

Actually, people considered removing the elevation advantage altogether.
One could see if the message is the Truth, and then one would remove the need for the elevation correction here.

So the most correct solution with the least effort would probably showing that updating units isn't so bad for performance.

We may consider adding these range overlays for siege engines btw.
In a23, the gameplay revolves a lot around siege attack range, so perhaps it would be a wise idea to add it.
It can just look ugly if there are too many range overlays overlapping - consider 30 selected women with their aura overlay.

Angen added a comment.Mar 26 2019, 8:35 PM

Yes, one would assume that when computation takes from 0 to 1 ms for one unit, it will take around x ms for x units but ( i was surprised ) it does not

fps (-> means was moving from one number to another, the most left is beginning, middle is mostly due doing selection itslef and most right is around which number +-2 frames is settled down)

112 units - no patch - overlay on - not moving - 50 -> 20 -> 38
112 units - no patch - overlay off - not moving - 50 -> 35 -> 45
112 units - no dpatch - overlay off - moving - 40
112 units - no patch - overlay on - moving - 30

112 units - update overlay on updateGUIObjects - not moving 50 -> 38 ( at already selected 40 fps)
112 units - update overlay on updateGUIObjects - moving - 40

112 units - update overlay on updateGUIObjects + elevation correction - not moving (selected) - 30 to 40
112 units - update overlay on updateGUIObjects + elevation correction - moving (selected) - 35

200 units - update overlay on updateGUIObjcets + elevation correction - not moving (selected) 15 to 20 (not selected 36)
200 units - update overlay on updateGUIObjects + elevation correction - moving (selected) 15 to 20 (not selected 34)

I measured created refreshEnabledRangeOverlays() function which did guiinterface call UpdateVisualRangeOverlay and this looped selected entities and updated overlay
to update 200 units it took from 5 to 7 ms. (rare occasion 8 and 9) even while moving them up the hill.

WARNING: 6ms
WARNING: 6ms
WARNING: 5ms
WARNING: 6ms
WARNING: 8ms
WARNING: 7ms
WARNING: 5ms
WARNING: 5ms
WARNING: 5ms
WARNING: 7ms
WARNING: 5ms
WARNING: 6ms
WARNING: 7ms
WARNING: 4ms
WARNING: 6ms
WARNING: 7ms
WARNING: 5ms
WARNING: 5ms
WARNING: 6ms
WARNING: 6ms
WARNING: 5ms
WARNING: 5ms
WARNING: 5ms
WARNING: 4ms
WARNING: 5ms
WARNING: 5ms
WARNING: 5ms
WARNING: 6ms
WARNING: 6ms
WARNING: 6ms
WARNING: 5ms
WARNING: 5ms
WARNING: 8ms
WARNING: 5ms
WARNING: 5ms
WARNING: 6ms
WARNING: 5ms
WARNING: 6ms
WARNING: 5ms
WARNING: 5ms
WARNING: 6ms
WARNING: 6ms
WARNING: 7ms
WARNING: 6ms
WARNING: 5ms
WARNING: 5ms
WARNING: 5ms
WARNING: 9ms
WARNING: 7ms
WARNING: 5ms
WARNING: 6ms
WARNING: 5ms
WARNING: 5ms

Thanks for the measuring.

In D1800#73539, @Angen wrote:

Yes, one would assume that when computation takes from 0 to 1 ms for one unit, it will take around x ms for x units but ( i was surprised ) it does not

But it's not more than Xms for X units, right? If many calls are 0 and some are 1, it can be anything between 0 and X. Can only find out the upper limit with that method.

fps (-> means was moving from one number to another, the most left is beginning, middle is mostly due doing selection itslef and most right is around which number +-2 frames is settled down)

I didn't comprehend

I measured created refreshEnabledRangeOverlays() function which did guiinterface call UpdateVisualRangeOverlay and this looped selected entities and updated overlay
to update 200 units it took from 5 to 7 ms. (rare occasion 8 and 9) even while moving them up the hill.

Was the function called upon position change?

If updating 200 range overlays of moving units each time they move only consumes 5-7ms per turn, that would be tolerable (even if it's 5-7% of a considerable future turn length).
If that is 5ms per update itself and the position changes are more than once per turn, we can't do it on position change, but could still update once per turn, or twice per second.

So it looks like we can find a way to always display it correctly one way or another.

Angen added a comment.EditedMar 27 2019, 5:45 AM

it is 5ms for all currently selected (200) units. I called this update from updateGUIObjects in sesson.js

112 units - no patch - overlay on - not moving - 50 -> 20 -> 38
I had around 50 fps before I made selection, when I did selection of 112 ranged units fps falled to 20 fps for second and then I had fps around 38

200 units - update overlay on updateGUIObjects + elevation correction - moving (selected) 15 to 20 (not selected 34)
I had selected 200 ranged units, when I was moving them and I kept them selected I got from 15 to 20 fps. When I ordered them to move and have them not selected I had 34 fps

All units have been moved in formation to reduce pathfinding fps drop. Overlays have not been updated on position change but of every selected unit when updateGUIObjects in sesson.js was called

Nescio added a subscriber: Nescio.Mar 27 2019, 11:30 AM

Restricting this update to buildings. For units this to work it needs to enable recomputing range overlay on unit motion what could become expensive.

Personally I would strongly recommend to correct it for all entities, including moving units, not just for structures.
At the moment the range overlays are incorrect, because they do not take the elevation bonus into account. Displaying incorrect information is worse than displaying no information at all.
Performance is of lesser importance; range overlays are options that can be turned off.

Performance is of lesser importance; range overlays are options that can be turned off.

Don't underestimate possible consequences of bad performance.
An simple aura bugfix in a23 made the code so slow that it consumed 10 seconds (i.e. 0 FPS, full freeze) when starting a game, that in turn disconnected most players of a match, aborting the game after people clicked on start, basically every time. That was terrible for the players.
You don't want freezes under any circumstance, and a freeze is nothing but CPU taking that long to compute the current operations.

Assume updating a range overlay would consume 100ms, but the units move all 50ms. Depending on how the renderer is programmed, it could lead to 0 FPS and players couldn't even open the options dialog to disable it.
Luckily the numbers seem acceptable.

Also, if range overlays are bad for performance to the player, the player likely won't know that it's the range overlays that should be disabled.
And if something is slow, fix it.

Otherwise I agree, the information shoud always be displayed correctly. It should be displayed correctly, and in a performant manner. Shouldn't tradeoff one defect against the other, but fix both.

All units have been moved in formation to reduce pathfinding fps drop.

I expect Pathfinding not to influence FPS, other than when the renderer waits for the simulation to complete the next turn (then it freezes for that time, 0 FPS. The displayed FPS is the average, so it can present itself as an FPS drop.)
Using formations supposedly improves simulation performance though and can reduce such freezes.
The freezes occur if the time to compute a turn exceeds the turn length I think.
That is the case if you have 8 players with 200 units each, but not with 1 player and 100 units running around.

Overlays have not been updated on position change but of every selected unit when updateGUIObjects in sesson.js was called

I recall that function was called often, but not sure how often.

FPS in general is not a good measure, because it depends a lot on the number of polygons displayed, thus on the camera location, on the camera angle,...
That's why measuring the time a piece of code needs is more informative.
If we know it consumes X microseconds to update a range overlay for one unit, and we know how often position change events can occur, then we can compute the worst case scenario.

If it consumes 5ms to update all range overlays, that would be tolerable I think.
It was proposed to reduce turn lengths to 100ms, so that would be 5% of a turn length. That's way too much, but it might still be tolerable, as it doesn't break the game fully and as Nescio mentioned, it's important not to display wrong information, and since that's the primary purpose of this patch, and since displaying 200 range overlays simultaneously is likely artistically not a good idea.

But I think we don't need to update the overlays on every position change, nor on every turn, twice per second would be good enough I suppose - then its 10ms / 1000ms.
I don't recall if the range overlays can be reset from the simulation, it would probably leave cleaner code if it would be located there as compared to the GUI. But I would have to look at the code and compare both ways to see which one would be less ugly if we can't update on every positionchange.

Angen removed a reviewer: Stan.May 3 2019, 10:15 AM
Angen planned changes to this revision.May 6 2019, 10:30 PM