Page MenuHomeWildfire Games

Move fauna_arctic_wolf_attack.xml from trigger/ to gaia/
AcceptedPublic

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

Details

Reviewers
elexis
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
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 7508
Build 12235: Vulcan BuildJenkins
Build 12234: arc lint + arc unit

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

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/.

elexis added a comment.EditedMay 18 2019, 11:06 AM

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.

elexis accepted this revision.May 18 2019, 8:43 PM

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