Paths

Table of Contentst

Differential D1904

ClosedPublic
Actions

Authored by Nescio on May 17 2019, 9:14 PM.

Details

Reviewers

elexis

Commits

rP22491: Move domestic wolf from templates/trigger/ to templates/gaia/, refs rP19323…

Summary

This patch moves fauna_arctic_wolf_attack.xml from trigger/ to gaia/ and corrects the two random map files that use it.
All other files in trigger/ inherit from template_trigger_point.xml, whereas fauna_arctic_wolf_attack.xml clearly does not:

<?xml version="1.0" encoding="utf-8"?>
<Entity parent="gaia/fauna_arctic_wolf">
  <Identity>
    <Classes datatype="tokens">Domestic</Classes>
  </Identity>
  <UnitAI>
    <NaturalBehaviour>violent</NaturalBehaviour>
  </UnitAI>
</Entity>

Test Plan

Quite frankly, I have no idea why it's put in trigger/.

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

Nescio created this revision.May 17 2019, 9:14 PM

Owners added a subscriber: Restricted Owners Package.May 17 2019, 9:14 PM

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/polar_sea.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/polar_sea.js
| 134| 134| g_Map.log("Creating dirt patches");
| 135| 135| createLayeredPatches(
| 136| 136| 	[scaleByMapSize(3, 6), scaleByMapSize(5, 10), scaleByMapSize(8, 21)],
| 137|    |-	[[tDirt,tHalfSnow], [tHalfSnow,tSnowLimited]],
|    | 137|+	[[tDirt, tHalfSnow], [tHalfSnow,tSnowLimited]],
| 138| 138| 	[2],
| 139| 139| 	avoidClasses(clWater, 3, clDirt, 5, clPlayer, 12),
| 140| 140| 	scaleByMapSize(15, 45),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/polar_sea.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/polar_sea.js
| 134| 134| g_Map.log("Creating dirt patches");
| 135| 135| createLayeredPatches(
| 136| 136| 	[scaleByMapSize(3, 6), scaleByMapSize(5, 10), scaleByMapSize(8, 21)],
| 137|    |-	[[tDirt,tHalfSnow], [tHalfSnow,tSnowLimited]],
|    | 137|+	[[tDirt,tHalfSnow], [tHalfSnow, tSnowLimited]],
| 138| 138| 	[2],
| 139| 139| 	avoidClasses(clWater, 3, clDirt, 5, clPlayer, 12),
| 140| 140| 	scaleByMapSize(15, 45),
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 0 tabs but found 1.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/polar_sea.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/polar_sea.js
| 150| 150| Engine.SetProgress(70);
| 151| 151| 
| 152| 152| g_Map.log("Creating stone mines");
| 153|    |-	createMines(
|    | 153|+createMines(
| 154| 154| 	[
| 155| 155| 		[new SimpleObject(oStoneSmall, 0, 2, 0, 4, 0, 2 * Math.PI, 1), new SimpleObject(oStoneLarge, 1, 1, 0, 4, 0, 2 * Math.PI, 4)],
| 156| 156| 		[new SimpleObject(oStoneSmall, 2,5, 1,3)]
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/polar_sea.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/polar_sea.js
| 153| 153| 	createMines(
| 154| 154| 	[
| 155| 155| 		[new SimpleObject(oStoneSmall, 0, 2, 0, 4, 0, 2 * Math.PI, 1), new SimpleObject(oStoneLarge, 1, 1, 0, 4, 0, 2 * Math.PI, 4)],
| 156|    |-		[new SimpleObject(oStoneSmall, 2,5, 1,3)]
|    | 156|+		[new SimpleObject(oStoneSmall, 2, 5, 1,3)]
| 157| 157| 	],
| 158| 158| 	avoidClasses(clWater, 3, clPlayer, 20, clRock, 18, clHill, 2),
| 159| 159| 	clRock);
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/polar_sea.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/polar_sea.js
| 153| 153| 	createMines(
| 154| 154| 	[
| 155| 155| 		[new SimpleObject(oStoneSmall, 0, 2, 0, 4, 0, 2 * Math.PI, 1), new SimpleObject(oStoneLarge, 1, 1, 0, 4, 0, 2 * Math.PI, 4)],
| 156|    |-		[new SimpleObject(oStoneSmall, 2,5, 1,3)]
|    | 156|+		[new SimpleObject(oStoneSmall, 2,5, 1, 3)]
| 157| 157| 	],
| 158| 158| 	avoidClasses(clWater, 3, clPlayer, 20, clRock, 18, clHill, 2),
| 159| 159| 	clRock);
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/polar_sea.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/polar_sea.js
| 161| 161| g_Map.log("Creating metal mines");
| 162| 162| createMines(
| 163| 163| 	[
| 164|    |-		[new SimpleObject(oMetalLarge, 1,1, 0,4)]
|    | 164|+		[new SimpleObject(oMetalLarge, 1, 1, 0,4)]
| 165| 165| 	],
| 166| 166| 	avoidClasses(clWater, 3, clPlayer, 20, clMetal, 18, clRock, 5, clHill, 2),
| 167| 167| 	clMetal);
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/polar_sea.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/public/maps/random/polar_sea.js
| 161| 161| g_Map.log("Creating metal mines");
| 162| 162| createMines(
| 163| 163| 	[
| 164|    |-		[new SimpleObject(oMetalLarge, 1,1, 0,4)]
|    | 164|+		[new SimpleObject(oMetalLarge, 1,1, 0, 4)]
| 165| 165| 	],
| 166| 166| 	avoidClasses(clWater, 3, clPlayer, 20, clMetal, 18, clRock, 5, clHill, 2),
| 167| 167| 	clMetal);
Executing section cli...

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

Harbormaster completed remote builds in B7508: Diff 8048.May 17 2019, 9:15 PM

Quite frankly, I have no idea why it's put in trigger/.

Which information sources are available that might contain this information, and how could the relevant piece of information be located?

First you can check the commit that put it there.
Then the commit message links to a revision proposal.
One can check the IRClogs of these days too then, I don't recall whether it was discussed there as well http://irclogs.wildfiregames.com/

As far as I know, it was put into the trigger/ folder because it was spawned by a triggerscript, period.
(So unless I'm mistaken and there was more to it, you learning how to dig through commit history and reviews would be more fruitful on other topics.)

So that argument itself seems like a bad argument, since all the other templates can also be spawned by trigger scripts.

In the first version of the polar sea map with this template, the template didn't even exist, but UnitAI was changed to allow non-domesticated animals to receive move-orders.
But you see in the revision history for this template that we reverted that in favor of some AnimalAI plans.

This story resurfaced here again https://wildfiregames.com/forum/index.php?/topic/25948-capturable-animals/page/2/

If we speak about this template, we also have to consider that the map idea might be so weird that it could be removed.
The wolves were added because the map was entirely empty and I felt it needed something to compensate for that. Perhaps I was wrong.

However deleting the wolves also doesn't solve the problem how it would be sorted out if it wasn't deleted, or in a mod / map pack.

We will face the same problems if we implement the campaign mode, where every campaign may have custom templates.

So in the gaia folder, it certainly makes sense, yes.

The patch itself looks good too if we aren't harsh and delete the wolf.

So my above theory on the patch is either to be controverted by the fossil record and testing of the patch and then accepted.

Given how often there were concerns where someone posted "told you so" with or without a link to a previous reference, it's probably better to check these references regardless of us being sure that this patch must be good as is.

(Such things are easier if one is the only one working on some piece of code!)

What I did before proposing this patch was look up the file in the repository ( https://trac.wildfiregames.com/browser/ps/trunk/binaries/data/mods/public/simulation/templates/trigger/fauna_arctic_wolf_attack.xml ) and check its revision log from there. The earliest commit I could find was rP19323; however, I couldn't find an explanation there why it should be located in trigger/
Because it inherits from a fauna template, not from template_trigger_point.xml, its current location is quite confusing.

The earliest commit I could find was rP19323; however, I couldn't find an explanation there why it should be located in trigger/

Good! After identifying the commit that introduced a certain behavior, one can check the references to that commit for information on the reasoning of the commit, right?

I just read the reference again, and I could find an explanation why it was committed under the filename why it was committed, why committer committed that template and why the reviewer accepted it, but not why it should be located in trigger/.

Because it inherits from a fauna template, not from template_trigger_point.xml, its current location is quite confusing.

But this statement could even be made stronger and proven to be correct, right?
I mean it's not only confusing but wrong following the definition of the gaia folder? (TODO: have a purpose definition for the folders)

(Edit: But you becoming educated on how to find the information online self-sufficiently is more important to me than having this template sorted out)

Currently gaia/ contains files that inherit from template_bird.xml, template_gaia.xml, and template_unit_fauna.xml and trigger/ contains files that inherit from template_trigger_point.xml; this file (fauna_arctic_wolf_attack.xml) is the only exception.

That's all I found as towards the reasoning why it's in that folder https://code.wildfiregames.com/D176#7385, so I guess there was no superseding reason to have it in trigger/.

I remember the template only occurs in this map, which means the patch is either correct, or if the patch is still wrong, noone can't say tolduso (at least there was no retrievable reason posted.)

Hence no choice but to accept this now. I guess you succeeded to make me an offer I can't refuse!

This revision is now accepted and ready to land.May 18 2019, 8:43 PM

Or fauna_arctic_wolf_domestic.xml?

Or Or fauna_arctic_wolf_violent.xml? (Other wolves are merely aggressive.)

In D1904#87049, @Nescio wrote:

Or Or fauna_arctic_wolf_violent.xml? (Other wolves are merely aggressive.)

But the domestic part is the (1) unexpected part, because wolves are entirely not domesticated, and (2) it's the most relevant part of the unit, because that's what allows them to receive orders, even the reason why the template exists.
As long as that implementation is the one in place, it would be IMO better to highlight this aspect.

Moved to gaia/fauna_arctic_wolf_domestic.xml

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

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/polar_sea.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/polar_sea.js
| 134| 134| g_Map.log("Creating dirt patches");
| 135| 135| createLayeredPatches(
| 136| 136| 	[scaleByMapSize(3, 6), scaleByMapSize(5, 10), scaleByMapSize(8, 21)],
| 137|    |-	[[tDirt,tHalfSnow], [tHalfSnow,tSnowLimited]],
|    | 137|+	[[tDirt, tHalfSnow], [tHalfSnow,tSnowLimited]],
| 138| 138| 	[2],
| 139| 139| 	avoidClasses(clWater, 3, clDirt, 5, clPlayer, 12),
| 140| 140| 	scaleByMapSize(15, 45),
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/polar_sea.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/polar_sea.js
| 134| 134| g_Map.log("Creating dirt patches");
| 135| 135| createLayeredPatches(
| 136| 136| 	[scaleByMapSize(3, 6), scaleByMapSize(5, 10), scaleByMapSize(8, 21)],
| 137|    |-	[[tDirt,tHalfSnow], [tHalfSnow,tSnowLimited]],
|    | 137|+	[[tDirt,tHalfSnow], [tHalfSnow, tSnowLimited]],
| 138| 138| 	[2],
| 139| 139| 	avoidClasses(clWater, 3, clDirt, 5, clPlayer, 12),
| 140| 140| 	scaleByMapSize(15, 45),
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 0 tabs but found 1.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/polar_sea.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/polar_sea.js
| 150| 150| Engine.SetProgress(70);
| 151| 151| 
| 152| 152| g_Map.log("Creating stone mines");
| 153|    |-	createMines(
|    | 153|+createMines(
| 154| 154| 	[
| 155| 155| 		[new SimpleObject(oStoneSmall, 0, 2, 0, 4, 0, 2 * Math.PI, 1), new SimpleObject(oStoneLarge, 1, 1, 0, 4, 0, 2 * Math.PI, 4)],
| 156| 156| 		[new SimpleObject(oStoneSmall, 2,5, 1,3)]
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/polar_sea.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/polar_sea.js
| 153| 153| 	createMines(
| 154| 154| 	[
| 155| 155| 		[new SimpleObject(oStoneSmall, 0, 2, 0, 4, 0, 2 * Math.PI, 1), new SimpleObject(oStoneLarge, 1, 1, 0, 4, 0, 2 * Math.PI, 4)],
| 156|    |-		[new SimpleObject(oStoneSmall, 2,5, 1,3)]
|    | 156|+		[new SimpleObject(oStoneSmall, 2, 5, 1,3)]
| 157| 157| 	],
| 158| 158| 	avoidClasses(clWater, 3, clPlayer, 20, clRock, 18, clHill, 2),
| 159| 159| 	clRock);
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/polar_sea.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/polar_sea.js
| 153| 153| 	createMines(
| 154| 154| 	[
| 155| 155| 		[new SimpleObject(oStoneSmall, 0, 2, 0, 4, 0, 2 * Math.PI, 1), new SimpleObject(oStoneLarge, 1, 1, 0, 4, 0, 2 * Math.PI, 4)],
| 156|    |-		[new SimpleObject(oStoneSmall, 2,5, 1,3)]
|    | 156|+		[new SimpleObject(oStoneSmall, 2,5, 1, 3)]
| 157| 157| 	],
| 158| 158| 	avoidClasses(clWater, 3, clPlayer, 20, clRock, 18, clHill, 2),
| 159| 159| 	clRock);
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/polar_sea.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/polar_sea.js
| 161| 161| g_Map.log("Creating metal mines");
| 162| 162| createMines(
| 163| 163| 	[
| 164|    |-		[new SimpleObject(oMetalLarge, 1,1, 0,4)]
|    | 164|+		[new SimpleObject(oMetalLarge, 1, 1, 0,4)]
| 165| 165| 	],
| 166| 166| 	avoidClasses(clWater, 3, clPlayer, 20, clMetal, 18, clRock, 5, clHill, 2),
| 167| 167| 	clMetal);
|    | [NORMAL] ESLintBear (comma-spacing):
|    | A space is required after ','.
|----|    | /zpool0/trunk/binaries/data/mods/public/maps/random/polar_sea.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/maps/random/polar_sea.js
| 161| 161| g_Map.log("Creating metal mines");
| 162| 162| createMines(
| 163| 163| 	[
| 164|    |-		[new SimpleObject(oMetalLarge, 1,1, 0,4)]
|    | 164|+		[new SimpleObject(oMetalLarge, 1,1, 0, 4)]
| 165| 165| 	],
| 166| 166| 	avoidClasses(clWater, 3, clPlayer, 20, clMetal, 18, clRock, 5, clHill, 2),
| 167| 167| 	clMetal);
Executing section cli...

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

Harbormaster completed remote builds in B8417: Diff 8953.Jul 17 2019, 3:46 PM

Wasn't there a related ticket? I only find #4770.

No idea; I stumbled upon this when viewing triggers/ files because of territory pull discussion.

I will commit this for the purpose of harmonization / putting all gaia animals into the same folder instead of having a stray folder with a stray file.

(If you're interested, D1881 is similar, since all other wallsets are inside structures/.)

Closed by commit rP22491: Move domestic wolf from templates/trigger/ to templates/gaia/, refs rP19323… (authored by elexis). · Explain WhyJul 17 2019, 4:15 PM

This revision was automatically updated to reflect the committed changes.