Page MenuHomeWildfire Games

Move the ability to hold a turret to a separate file.
ClosedPublic

Authored by Freagarach on Oct 11 2019, 10:26 PM.

Details

Reviewers
wraitii
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23856: Move the ability to hold a turret to a separate file.
Summary

This moves the logic to garrison visibly to a seperate file, to allow it to be used in other context (e.g. being able to tell which units ought to be visibly garrisoned).
With a tad more effort one might even be able to task a turret to "turn in", i.e. go from a visibly garrisoned state to a hidden one.
This could be used in the functionality mentioned in #3488.

Extension:

  • Be able to use a TurretHolder without GarrisonHolder (needs some GUI adaptations, perhaps).

As a sideeffect this allows entities without position to garrison, which is strange, but doable now.

Test Plan

Verify that normal (un)garrisoning works as it did before by trying stuff:

  • (Un)garrison units in a CC.
  • (Un)garrison units on(/from) a wall-segment.
  • Autogarrisoning.
  • Trying a map with garrisoned entities.
  • Try promoting entities garrisoned (visible).

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

This is a good idea, but then again it's my suggestion so that's cheating :P

Current direction seems quite good. I think you can cleanup the messages in GarrisonHolder as said inline, but nothing else too obvious strikes me. I'm actually surprised that UnitAI needs no changes.
Beyond the tests, I'm not sure what else you consider WIP here.

Disclaimer -> not actually tested.
Also might need a slight rebase on top of rP23710 but not sure.

binaries/data/mods/public/simulation/components/GarrisonHolder.js
161 ↗(On Diff #12038)

Seems like this should go in a "TurretChange" message or something, depending on what the calling code is doing with it.

Edit -> you've already added one, so I further question this

174 ↗(On Diff #12038)

Better to MoveOutOfWorld if we have a position than force composition.

You could completely imagine garrisoning ethereal things that don't actually have a position in the world.

binaries/data/mods/public/simulation/components/TurretHolder.js
3 ↗(On Diff #12038)

I don't think the ability to move is per se a requirement of turreting.

86 ↗(On Diff #12038)

This looks like a workaround to some undisclosed issue. What is the issue, why does this fix it?

102 ↗(On Diff #12038)

Just 'leave" perhaps.

136 ↗(On Diff #12038)

could also add the turretPoint hint here but OK

Freagarach marked 2 inline comments as done.Jun 4 2020, 8:59 AM

The reason this is [WIP] is that one still needs GarrisonHolder in order to occupy a turret. That is also why UnitAI does not need changes.
Complete separation probably means duplication of the eject logic (although that can be saved for a future patch?).

The reason this is [WIP] is that one still needs GarrisonHolder in order to occupy a turret. That is also why UnitAI does not need changes.
Complete separation probably means duplication of the eject logic (although that can be saved for a future patch?).

That seems to only be because only GarrisonHolder code sets turrets, but from what I see you could add other code that handles it for non-ejectable turrets ? Triggers also could do it already.

IMO definitely for another patch.

Freagarach planned changes to this revision.Jun 4 2020, 9:05 AM

Cool :)
I still need to do the inlines though.

Freagarach updated this revision to Diff 12178.Jun 6 2020, 5:21 PM
Freagarach marked 5 inline comments as done.
Freagarach retitled this revision from [WIP] - Move the ability to hold a turret to a separate file. to Move the ability to hold a turret to a separate file..
Freagarach edited the summary of this revision. (Show Details)
Freagarach edited the test plan for this revision. (Show Details)

Inlines.

Vulcan added a comment.Jun 6 2020, 5:22 PM

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

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

Freagarach added inline comments.Jun 6 2020, 5:27 PM
binaries/data/mods/public/simulation/components/GarrisonHolder.js
161 ↗(On Diff #12038)

This is to add arrows to BuildingAI or not. If GarrisonHolder and TurretHolder are compleatly split this would not be needed anymore.

binaries/data/mods/public/simulation/components/TurretHolder.js
86 ↗(On Diff #12038)
Vulcan added a comment.Jun 6 2020, 5:32 PM

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

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

Stan added inline comments.Jun 6 2020, 5:40 PM
binaries/data/mods/public/simulation/components/TurretHolder.js
107 ↗(On Diff #12179)

array.Find?

117 ↗(On Diff #12179)

Inconsistent variable name. Ent/Entity. Maybe we could do without Ent/Entity

binaries/data/mods/public/simulation/components/tests/test_TurretHolder.js
47 ↗(On Diff #12179)

Strings and decimals maybe :)

92 ↗(On Diff #12179)

TODO: ?

wraitii added inline comments.Jun 6 2020, 5:43 PM
binaries/data/mods/public/simulation/components/GarrisonHolder.js
189 ↗(On Diff #12179)

What is preventing the "complete split" here?

Freagarach updated this revision to Diff 12191.Jun 7 2020, 8:52 AM
Freagarach marked 6 inline comments as done.
  • Inlines.
  • Expanded test.
Vulcan added a comment.Jun 7 2020, 8:53 AM

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

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

Freagarach added inline comments.Jun 7 2020, 8:54 AM
binaries/data/mods/public/simulation/components/GarrisonHolder.js
189 ↗(On Diff #12179)

That currently turreted entities are also garrisoned and turreted entities may not count for the garrisoned arrows.

wraitii added inline comments.Jun 7 2020, 9:04 AM
binaries/data/mods/public/simulation/components/GarrisonHolder.js
189 ↗(On Diff #12179)

My question was more along the lines of

You're querying the turret Holder component here. Why not query it in BuildingAI when parsing a GarrisonedUnitsChanged instead?

Freagarach marked an inline comment as done.Jun 7 2020, 9:11 AM
Freagarach added inline comments.
binaries/data/mods/public/simulation/components/GarrisonHolder.js
189 ↗(On Diff #12179)

Because one would need to query it twice, here and in BuildingAI. Also when the split would be compleat one needs to touch BuildingAI again to remove it. But mainly the former ;)

Freagarach marked an inline comment as done.Jun 7 2020, 9:11 AM
wraitii added inline comments.Jun 7 2020, 9:48 AM
binaries/data/mods/public/simulation/components/GarrisonHolder.js
189 ↗(On Diff #12179)

This wouldn't be a problem if you listened to OnGarrisonedUnitsChanged messages in TurretHolder and thus handled this entirely in the turretHolder component, but I guess that depends on D2644.

That is a rather annoying dependency, I think I'll try and get it moving.

Freagarach planned changes to this revision.Jun 7 2020, 10:58 AM

Breaks when promoting the second visibly garrisoned entity if the first has left.

Freagarach updated this revision to Diff 12215.Jun 8 2020, 9:05 AM

Fix renaming entities.

Vulcan added a comment.Jun 8 2020, 9:07 AM

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

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

wraitii requested changes to this revision.Jun 14 2020, 10:39 AM

Tested this and it seems to work properly.

Requesting changes over the renamed thing, I think there's a slightly cleaner way to handle this. After that, I believe this is good to go.

binaries/data/mods/public/simulation/components/BuildingAI.js
39 ↗(On Diff #12215)

TBH I think it's OK, it's probably almost as fast.
I'd make the comment a tad more explicit, something like "Garrisoned units may also be occupying turrets, in which case they don't count toward arrows".

binaries/data/mods/public/simulation/components/GarrisonHolder.js
253 ↗(On Diff #12215)

L234 would be better above here I think

535 ↗(On Diff #12215)

What I would do here is make the "renamed" argument into a "Quiet" argument, which we already have on some other components. If "quiet" is true, you don't send the message.
Then the logic becomes:

this.Eject(msg.entity, true, true);
if (!this.Garrison(msg.newentity, true))
    Engine.PostMessage(MT_GarrisonedUnitsChanged, ...)

And you can just let the turretHolder subscribe to renamed messages (obviously pending D2644 but I think I'll commit that soon).

I think it's OK because other component might still listen to OnRenamed if they handle entities, and arguably the garrisoned units haven't changed, so semantically sending the message is buggy.


Also I think your current implementation isn't working if the entity-upgraded-to couldn't garrison

binaries/data/mods/public/simulation/components/TurretHolder.js
177 ↗(On Diff #12215)

Rename this to "SwapEntity" or something, and it'll look less weird and could be reused for other things in the future

This revision now requires changes to proceed.Jun 14 2020, 10:39 AM
wraitii added inline comments.Jun 14 2020, 10:40 AM
binaries/data/mods/public/simulation/components/TurretHolder.js
177 ↗(On Diff #12215)

TBH this was pre-my-change-idea, so if you just make this OnGlobalEntityRenamed it's fine.

Freagarach updated this revision to Diff 12327.Jun 15 2020, 7:46 PM
Freagarach marked 2 inline comments as done.

Send message when turrets change and react upon that in BuildingAI.

Notice that renaming an turreted entity to an entity which is not allowed to garrison at all is broken, before and after this patch.

Freagarach added inline comments.Jun 15 2020, 8:02 PM
binaries/data/mods/public/simulation/components/GarrisonHolder.js
517 ↗(On Diff #12327)

To be removed.

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

Linter detected issues:
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/tests/test_TurretHolder.js
|  30| »   »   "SetTurretParent":·(entity,·offset)·=>·{},
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'entity' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/TurretHolder.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/TurretHolder.js
| 191| 191| 		if (turretPoint)
| 192| 192| 			this.LeaveTurret(msg.entity, turretPoint);
| 193| 193| 
| 194|    |-		let cmpGarrisonHolder = Engine.QueryInterface(this.entity, IID_GarrisonHolder)
|    | 194|+		let cmpGarrisonHolder = Engine.QueryInterface(this.entity, IID_GarrisonHolder);
| 195| 195| 		if (cmpGarrisonHolder && cmpGarrisonHolder.IsGarrisoned(msg.newentity))
| 196| 196| 			this.OccupyTurret(msg.newentity, turretPoint);
| 197| 197| 	}

binaries/data/mods/public/simulation/components/TurretHolder.js
| 194| »   »   let·cmpGarrisonHolder·=·Engine.QueryInterface(this.entity,·IID_GarrisonHolder)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/GuiInterface.js
| 358| 358| 			"garrisonedEntitiesCount": cmpGarrisonHolder.GetGarrisonedEntitiesCount()
| 359| 359| 		};
| 360| 360| 
| 361|    |-	let cmpTurretHolder = Engine.QueryInterface(ent, IID_TurretHolder)
|    | 361|+	let cmpTurretHolder = Engine.QueryInterface(ent, IID_TurretHolder);
| 362| 362| 	if (cmpTurretHolder)
| 363| 363| 		ret.turretHolder = {
| 364| 364| 			"turretPoints": cmpTurretHolder.GetTurretPoints()

binaries/data/mods/public/simulation/components/GuiInterface.js
| 361| »   let·cmpTurretHolder·=·Engine.QueryInterface(ent,·IID_TurretHolder)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
| 139| 139| 
| 140| 140| 	var range = cmpAttack.GetRange(attackType);
| 141| 141| 	this.enemyUnitsQuery = cmpRangeManager.CreateActiveParabolicQuery(
| 142|    |-			this.entity, range.min, range.max, range.elevationBonus,
|    | 142|+		this.entity, range.min, range.max, range.elevationBonus,
| 143| 143| 			enemies, IID_Resistance, cmpRangeManager.GetEntityFlagMask("normal"));
| 144| 144| 
| 145| 145| 	cmpRangeManager.EnableActiveQuery(this.enemyUnitsQuery);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
| 140| 140| 	var range = cmpAttack.GetRange(attackType);
| 141| 141| 	this.enemyUnitsQuery = cmpRangeManager.CreateActiveParabolicQuery(
| 142| 142| 			this.entity, range.min, range.max, range.elevationBonus,
| 143|    |-			enemies, IID_Resistance, cmpRangeManager.GetEntityFlagMask("normal"));
|    | 143|+		enemies, IID_Resistance, cmpRangeManager.GetEntityFlagMask("normal"));
| 144| 144| 
| 145| 145| 	cmpRangeManager.EnableActiveQuery(this.enemyUnitsQuery);
| 146| 146| };
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
| 168| 168| 
| 169| 169| 	// This query is only interested in Gaia entities that can attack.
| 170| 170| 	this.gaiaUnitsQuery = cmpRangeManager.CreateActiveParabolicQuery(
| 171|    |-			this.entity, range.min, range.max, range.elevationBonus,
|    | 171|+		this.entity, range.min, range.max, range.elevationBonus,
| 172| 172| 			[0], IID_Attack, cmpRangeManager.GetEntityFlagMask("normal"));
| 173| 173| 
| 174| 174| 	cmpRangeManager.EnableActiveQuery(this.gaiaUnitsQuery);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
| 169| 169| 	// This query is only interested in Gaia entities that can attack.
| 170| 170| 	this.gaiaUnitsQuery = cmpRangeManager.CreateActiveParabolicQuery(
| 171| 171| 			this.entity, range.min, range.max, range.elevationBonus,
| 172|    |-			[0], IID_Attack, cmpRangeManager.GetEntityFlagMask("normal"));
|    | 172|+		[0], IID_Attack, cmpRangeManager.GetEntityFlagMask("normal"));
| 173| 173| 
| 174| 174| 	cmpRangeManager.EnableActiveQuery(this.gaiaUnitsQuery);
| 175| 175| };
Executing section cli...

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

Freagarach updated this revision to Diff 12339.Jun 16 2020, 5:11 PM
  • Typo.
  • Remove unnecessary TurretHolder-queries in GarrisonHolder.js.

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

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

You haven't really reacted to my suggestion here, and in fact I'm also repeating my comment for SwapEntity.
Maybe that was on purpose, maybe not, but I'd like to understand your reasoning :)

I think the 'renamed' thing is less clean than a 'quiet' flag that doesn't send the message at all. Perhaps the garrisoning message should be "GarrisonedSlotChanged" in that case.

binaries/data/mods/public/simulation/components/TurretHolder.js
188 ↗(On Diff #12339)

I think you should rename this to SwapEntity and pass IDs directly.

From our discussion above, I don't think it can be removed tomorrow :P

Likewise, I think it would be good to clarify in the comment "ToDo" what is necessary to make it not needed.

Freagarach planned changes to this revision.Jul 17 2020, 12:15 PM
Freagarach added inline comments.
binaries/data/mods/public/simulation/components/GarrisonHolder.js
535 ↗(On Diff #12215)

The problem is that the garrisoning units have in fact changed so IMHO it is bad to not send the message.
There are other components which rely on both the ejected and subsequent garrison messages.
To fix those one needs to listen to global entity renames, which would be expensive to do (e.g. every entity that has cmpCapturable or cmpAuras).

binaries/data/mods/public/simulation/components/TurretHolder.js
188 ↗(On Diff #12339)

Thanks, good one.

Freagarach updated this revision to Diff 12750.Jul 17 2020, 1:06 PM
Freagarach marked an inline comment as done.
Freagarach edited the test plan for this revision. (Show Details)
  • EntityRenamed -> SwapEntities.
  • Clarify comment a tad.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/TurretHolder.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/TurretHolder.js
| 194| 194| 		if (turretPoint)
| 195| 195| 			this.LeaveTurret(from, turretPoint);
| 196| 196| 
| 197|    |-		let cmpGarrisonHolder = Engine.QueryInterface(this.entity, IID_GarrisonHolder)
|    | 197|+		let cmpGarrisonHolder = Engine.QueryInterface(this.entity, IID_GarrisonHolder);
| 198| 198| 		if (cmpGarrisonHolder && cmpGarrisonHolder.IsGarrisoned(to))
| 199| 199| 			this.OccupyTurret(to, turretPoint);
| 200| 200| 	}

binaries/data/mods/public/simulation/components/TurretHolder.js
| 197| »   »   let·cmpGarrisonHolder·=·Engine.QueryInterface(this.entity,·IID_GarrisonHolder)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.

binaries/data/mods/public/simulation/components/tests/test_TurretHolder.js
|  30| »   »   "SetTurretParent":·(entity,·offset)·=>·{},
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'entity' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
| 139| 139| 
| 140| 140| 	var range = cmpAttack.GetRange(attackType);
| 141| 141| 	this.enemyUnitsQuery = cmpRangeManager.CreateActiveParabolicQuery(
| 142|    |-			this.entity, range.min, range.max, range.elevationBonus,
|    | 142|+		this.entity, range.min, range.max, range.elevationBonus,
| 143| 143| 			enemies, IID_Resistance, cmpRangeManager.GetEntityFlagMask("normal"));
| 144| 144| 
| 145| 145| 	cmpRangeManager.EnableActiveQuery(this.enemyUnitsQuery);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
| 140| 140| 	var range = cmpAttack.GetRange(attackType);
| 141| 141| 	this.enemyUnitsQuery = cmpRangeManager.CreateActiveParabolicQuery(
| 142| 142| 			this.entity, range.min, range.max, range.elevationBonus,
| 143|    |-			enemies, IID_Resistance, cmpRangeManager.GetEntityFlagMask("normal"));
|    | 143|+		enemies, IID_Resistance, cmpRangeManager.GetEntityFlagMask("normal"));
| 144| 144| 
| 145| 145| 	cmpRangeManager.EnableActiveQuery(this.enemyUnitsQuery);
| 146| 146| };
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
| 168| 168| 
| 169| 169| 	// This query is only interested in Gaia entities that can attack.
| 170| 170| 	this.gaiaUnitsQuery = cmpRangeManager.CreateActiveParabolicQuery(
| 171|    |-			this.entity, range.min, range.max, range.elevationBonus,
|    | 171|+		this.entity, range.min, range.max, range.elevationBonus,
| 172| 172| 			[0], IID_Attack, cmpRangeManager.GetEntityFlagMask("normal"));
| 173| 173| 
| 174| 174| 	cmpRangeManager.EnableActiveQuery(this.gaiaUnitsQuery);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
| 169| 169| 	// This query is only interested in Gaia entities that can attack.
| 170| 170| 	this.gaiaUnitsQuery = cmpRangeManager.CreateActiveParabolicQuery(
| 171| 171| 			this.entity, range.min, range.max, range.elevationBonus,
| 172|    |-			[0], IID_Attack, cmpRangeManager.GetEntityFlagMask("normal"));
|    | 172|+		[0], IID_Attack, cmpRangeManager.GetEntityFlagMask("normal"));
| 173| 173| 
| 174| 174| 	cmpRangeManager.EnableActiveQuery(this.gaiaUnitsQuery);
| 175| 175| };
|    | [NORMAL] ESLintBear (semi):
|    | Missing semicolon.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/GuiInterface.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/GuiInterface.js
| 359| 359| 			"garrisonedEntitiesCount": cmpGarrisonHolder.GetGarrisonedEntitiesCount()
| 360| 360| 		};
| 361| 361| 
| 362|    |-	let cmpTurretHolder = Engine.QueryInterface(ent, IID_TurretHolder)
|    | 362|+	let cmpTurretHolder = Engine.QueryInterface(ent, IID_TurretHolder);
| 363| 363| 	if (cmpTurretHolder)
| 364| 364| 		ret.turretHolder = {
| 365| 365| 			"turretPoints": cmpTurretHolder.GetTurretPoints()

binaries/data/mods/public/simulation/components/GuiInterface.js
| 362| »   let·cmpTurretHolder·=·Engine.QueryInterface(ent,·IID_TurretHolder)
|    | [NORMAL] JSHintBear:
|    | Missing semicolon.
Executing section cli...

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

wraitii accepted this revision.Jul 17 2020, 2:03 PM

Functional in my tests, unit_demo works, prepped for GarrisonPoint and found nothing.

This is a breaking change for some mods, so could be worth informing on the forum thread:


Very good change overall though, thanks for the hard work on this.

binaries/data/mods/public/simulation/components/GarrisonHolder.js
535 ↗(On Diff #12215)

The problem is that the garrisoning units have in fact changed so IMHO it is bad to not send the message.

This is arguable and kind of philosophical, hence my 'slot' command. I think semantically subscribing to 'garrisonslots' and 'entity renamed' makes sense, but I agree with the performance concern.

Fine for me then.

539 ↗(On Diff #12750)

This also isn't very obvious.

My take:

TurretHolder is not subscribed to GarrisonChanged, so we must inform it explicitly.
Otherwise a renaming entity will free a turret (which may then be occupied by another), and then re-occupy another turret, which is not wanted.
This revision is now accepted and ready to land.Jul 17 2020, 2:03 PM
Freagarach marked 6 inline comments as done.
  • Linter issues.
  • Update comment for separate renaming by TurretHolder.

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
| 139| 139| 
| 140| 140| 	var range = cmpAttack.GetRange(attackType);
| 141| 141| 	this.enemyUnitsQuery = cmpRangeManager.CreateActiveParabolicQuery(
| 142|    |-			this.entity, range.min, range.max, range.elevationBonus,
|    | 142|+		this.entity, range.min, range.max, range.elevationBonus,
| 143| 143| 			enemies, IID_Resistance, cmpRangeManager.GetEntityFlagMask("normal"));
| 144| 144| 
| 145| 145| 	cmpRangeManager.EnableActiveQuery(this.enemyUnitsQuery);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
| 140| 140| 	var range = cmpAttack.GetRange(attackType);
| 141| 141| 	this.enemyUnitsQuery = cmpRangeManager.CreateActiveParabolicQuery(
| 142| 142| 			this.entity, range.min, range.max, range.elevationBonus,
| 143|    |-			enemies, IID_Resistance, cmpRangeManager.GetEntityFlagMask("normal"));
|    | 143|+		enemies, IID_Resistance, cmpRangeManager.GetEntityFlagMask("normal"));
| 144| 144| 
| 145| 145| 	cmpRangeManager.EnableActiveQuery(this.enemyUnitsQuery);
| 146| 146| };
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
| 168| 168| 
| 169| 169| 	// This query is only interested in Gaia entities that can attack.
| 170| 170| 	this.gaiaUnitsQuery = cmpRangeManager.CreateActiveParabolicQuery(
| 171|    |-			this.entity, range.min, range.max, range.elevationBonus,
|    | 171|+		this.entity, range.min, range.max, range.elevationBonus,
| 172| 172| 			[0], IID_Attack, cmpRangeManager.GetEntityFlagMask("normal"));
| 173| 173| 
| 174| 174| 	cmpRangeManager.EnableActiveQuery(this.gaiaUnitsQuery);
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/simulation/components/BuildingAI.js
| 169| 169| 	// This query is only interested in Gaia entities that can attack.
| 170| 170| 	this.gaiaUnitsQuery = cmpRangeManager.CreateActiveParabolicQuery(
| 171| 171| 			this.entity, range.min, range.max, range.elevationBonus,
| 172|    |-			[0], IID_Attack, cmpRangeManager.GetEntityFlagMask("normal"));
|    | 172|+		[0], IID_Attack, cmpRangeManager.GetEntityFlagMask("normal"));
| 173| 173| 
| 174| 174| 	cmpRangeManager.EnableActiveQuery(this.gaiaUnitsQuery);
| 175| 175| };
Executing section cli...

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

This revision was automatically updated to reflect the committed changes.