Page MenuHomeWildfire Games

Move placeable templates out of special/.
ClosedPublic

Authored by leper on Sep 6 2017, 6:24 AM.

Details

Reviewers
elexis
FeXoR
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Owners Package(Owns No Changed Paths)
Commits
rP20181: Move placeable templates out of special/.
Summary

special/ shouldn't contain placeable templates. Currently this prevents us from switching the placeablesFilter to some unplaceablesFilter, which would make modders templates (in some new folder structure different from ours) show up in atlas by default.

Test Plan

Check that nothing breaks, notify modders about the change and help them do that (aka run sed).

If someone has a better place for the territory_pull template (possibly a new folder), I'd be happy to hear suggestions.

Event Timeline

leper created this revision.Sep 6 2017, 6:24 AM
Vulcan added a comment.Sep 6 2017, 7:10 AM
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/maps/random/polar_sea.js
| 193| »   let·terrainPainter·=·new·LayeredPainter(
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'terrainPainter' is already declared in the upper scope.

binaries/data/mods/public/maps/random/polar_sea.js
| 197| »   let·elevationPainter·=·new·SmoothElevationPainter(ELEVATION_SET,·-5,·5);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'elevationPainter' is already declared in the upper scope.
|    | [NORMAL] ESLintBear (no-multi-spaces):
|    | Multiple spaces found before 'i'.
|----|    | /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/maps/random/survivalofthefittest.js
|    |++++| /mnt/data/jenkins-phabricator/workspace/phabricator_lint/binaries/data/mods/public/maps/random/survivalofthefittest.js
|  87|  87| var playerAngle = new Array(numPlayers);
|  88|  88| 
|  89|  89| var startAngle = randFloat(0, 2 * PI);
|  90|    |-for (let  i = 0; i < numPlayers; ++i)
|    |  90|+for (let i = 0; i < numPlayers; ++i)
|  91|  91| {
|  92|  92| 	playerAngle[i] = startAngle + i * 2 * PI / numPlayers;
|  93|  93| 	playerX[i] = 0.5 + 0.3*cos(playerAngle[i]);

binaries/data/mods/public/maps/random/survivalofthefittest.js
|  90| for·(let··i·=·0;·i·<·numPlayers;·++i)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/maps/random/survivalofthefittest.js
|  99| for·(let·i·=·0;·i·<·numPlayers;·++i)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/maps/random/survivalofthefittest.js
| 117| »   let·ix·=·round(fx);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'ix' is already declared in the upper scope.

binaries/data/mods/public/maps/random/survivalofthefittest.js
| 118| »   let·iz·=·round(fz);
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'iz' is already declared in the upper scope.

binaries/data/mods/public/maps/random/danubius.js
| 247| »   »   »   placeCustomFortress(mX,·mZ,·new·Fortress("celt·ritual·males",·new·Array(maleCount).fill(0).map(i·=>
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/maps/random/danubius.js
| 307| »   »   for·(let·i·=·0;·i·<·gallicCCTreasureCount;·++i)
|    | [NORMAL] ESLintBear (no-shadow):
|    | 'i' is already declared in the upper scope.

binaries/data/mods/public/maps/random/danubius.js
| 177| »   »   let·gX·=·i·==·0·?·gaulCityBorderDistance·:·mapSize·-·gaulCityBorderDistance;
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/maps/random/danubius.js
| 186| »   »   »   let·mX·=·i·==·0·?
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/maps/random/danubius.js
| 197| »   »   »   »   gX·+·gaulCityRadius·*·(i·==·0·?·1·:·-1),
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/maps/random/danubius.js
| 247| »   »   »   placeCustomFortress(mX,·mZ,·new·Fortress("celt·ritual·males",·new·Array(maleCount).fill(0).map(i·=>
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/maps/random/danubius.js
| 331| »   if·(numPlayers·%·2·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/maps/random/danubius.js
| 618| »   »   i·==·0·?
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/maps/random/danubius.js
| 773| »   »   »   »   i·==·0·?·triggerPointShipUnloadLeft·:·triggerPointShipUnloadRight,
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/maps/random/danubius.js
| 791| »   »   »   »   i·==·0·?·triggerPointLandPatrolLeft·:·triggerPointLandPatrolRight,
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0

http://jenkins-master:8080/job/phabricator_lint/496/ for more details.

Stan added a subscriber: Stan.Sep 6 2017, 8:06 AM

I think the convention was that if you put a file in a folder containing the prefix you should remove the prefix from the file.

IE trigger_point_a.xml -> triggers/point_a.xml or triggers/points/a.xml

elexis added a subscriber: elexis.Sep 6 2017, 7:50 PM

(Phabricator doesn't support the moved files in that patch, svn patch neither.)

Review:

Trigger Points:
I definitely agree with the trigger point move. A search for trigger_point yielded the same files as modified in this patch, so unless theres a change missing in a single file, it is complete too.
(Wouldn't mind if that is committed, so that we can focus on the rest, or not.)

special/ and other/:

special/ shouldn't contain placeable templates

special and other are very undescriptive names and the difference is intransparent from that name.
developers and modders should be informed about the constraints of template directories.
So rename it to unplaceable afterwards? Sounds a bit weird, yet it carries some kind of connotation.
How about special_placeable and special_unplaceable for templates in other and special?

territory_pull:
That is definitely placeable file and placed on Campaign Test Map.xml.
So if special should not contain placeable templates, territory_pull must indeed be moved out of special.
Since I don't know about the other constraints, dunno if that is the right directory.
If you are sure it is correct and don't want to rename other to something more clear, then I can confirm that the patch is complete.

binaries/data/mods/public/maps/random/survivalofthefittest.js
42

I like the trigger directory. Makes the directory structure more transparent.

binaries/data/mods/public/maps/scenarios/Campaign Test Map.xml
2007

other is a bit meanlingless too. Would be great to have names that imply anything/everything the templates have in common.

elexis added a comment.Sep 6 2017, 7:56 PM

Move placeable templates out of special/

and I can confirm that the remaining files in special/ are not placeable.

actor.xml
player_cart.xml
player_iber.xml
player_pers.xml
player_sele.xml
rallypoint.xml
player_athen.xml
player_gaia.xml
player_mace.xml
player_ptol.xml
player_spart.xml
spy.xml
player_brit.xml
player_gaul.xml
player_maur.xml
player_rome.xml
player.xml
target_marker.xml

(actor only used for inheritance.)

The title doesn't state anything about moving unplaceable templates to special/, so not investigating that (Some unplaceables like template_unit definitely don't fit there).

In D877#34302, @elexis wrote:

(Phabricator doesn't support the moved files in that patch, svn patch neither.)

Not sure if git-svn will keep all the move history, not that that will make digging through the history a lot harder.

special and other are very undescriptive names and the difference is intransparent from that name.

Yes, other are just ones that don't really fit anywhere (mostly random buildings used on some maps).
Special are well special templates.

developers and modders should be informed about the constraints of template directories.

Mostly none at all, apart from some things not showing up in certain queries.
(Well, at least that's what it should be, currently there is that placeable filter thing that isn't really obvious.)

So rename it to unplaceable afterwards? Sounds a bit weird, yet it carries some kind of connotation.

Well special_units also contains unplaceable things, though those could be renamed to start with "template_".

How about special_placeable and special_unplaceable for templates in other and special?

Doesn't seem to add anything, and everything int special* would be unplaceable after this. (Same for "rubble", "formations", and "template_*".

If you are sure it is correct and don't want to rename other to something more clear, then I can confirm that the patch is complete.

Well those are just other placeable templates, no real categorization apart from that.

elexis accepted this revision.Sep 7 2017, 12:36 AM

The trigger part is correct independent of how we call special and other. Noone should have to see that diff anymore, so better flush it. The territory_pull part can be committed while at it, even if that campaign map has to changed in case we do rename those directories.

I'm still unhappy with other and special. Theres no connotation, no direction, no information in it. A more clear directory structure, possibly merging some directories, might also make the un/placeables filters shorter.

I meant renaming other to special_placeables and special to special_unplaceables. But any improvement to the transparency of the directory strucutre is welcome.

This revision is now accepted and ready to land.Sep 7 2017, 12:36 AM
FeXoR accepted this revision.Sep 8 2017, 6:09 PM
FeXoR added a subscriber: FeXoR.

I somehow agree with elexis meaningful folder naming.
Still I consider this is a welcome improvement ;)

FeXoR requested changes to this revision.EditedSep 8 2017, 6:15 PM

@Stan pointed out that since the trigger points are moved to the directory triggers the trigger prefix should be removed.
I agree since this was the default approach previously, too.
So this should be part of this commit to stay conform.

This revision now requires changes to proceed.Sep 8 2017, 6:15 PM
elexis added a comment.Sep 8 2017, 6:17 PM

Therefore we also remove the template_ prefix from the files in templates/?

FeXoR added a comment.Sep 8 2017, 6:39 PM

@elexis Ultimately, yes. But ATM there are 2 kinds of templates in the directory templates:

  • Definitions to generate entities from (in the subdirectories). Entity templates so to say
  • Perdefinitions of of a set of those (with the prefix template_), Entity template templates so to say.

Both are templates but in a different way.

So at least I don't think it should be part of this patch.

leper requested review of this revision.Sep 13 2017, 1:45 AM

I disagree with the conclusion of also renaming template_, and I don't really see the point in renaming those templates. If someone wants to change that, take over; you'll have to fix the breakage from the other ticket yourself though.

(While special and other do sound synonymous, by the directory contents it is clear that the special ones are not supposed to be placeable.)
(My question was rhetorical and my green checkmark is still valid.)

This revision was automatically updated to reflect the committed changes.

(My question was rhetorical and my green checkmark is still valid.)

(Yeah, got that part.)

How about special_placeable and special_unplaceable for templates in other and special?

Doesn't seem to add anything

Ignoring all my points about this folder being unclear.

special and other are very undescriptive names and the difference is intransparent from that name.
developers and modders should be informed about the constraints of template directories.
So rename it to unplaceable afterwards? Sounds a bit weird, yet it carries some kind of connotation.

everything in special* would be unplaceable after this

I did not presume code to be unchangeable and I don't expect you to either.

Special are well special templates.

Circular definitions are just that.

(by the directory contents it is clear that the special ones are not supposed to be placeable.)

Or not, see the three people in D1798 not figuring out the non-obvious common denominator.

Sorry to the general public for making the right observations, finding the right conclusions in the comments but then accepting it instead of requesting changes.
The filter introduced from seemingly rP7259 also didn't provide a description, so this commit isn't solely responsible for fallout.