Page MenuHomeWildfire Games

rename maps/ file names to use underscores (instead of spaces and capitalization)
Needs ReviewPublic

Authored by Nescio on Nov 15 2017, 1:34 PM.

Details

Reviewers
elexis
Summary

File names in the public folder typically use underscores; occassionally capitalization or CamelCase is used. Many of the maps files are different in this respect: many include spaces (" ") in their file names, which is not only inconsistent, but also inconvenient when using a command line.

The situation currently is:
maps/random/: all file names use underscores and no capitalization
maps/scenarios/: about half use spaces and capitalization, others underscores, a few camel case
maps/skirmishes/: most use spaces and capitalization, a few underscores

To implement the consistent renaming, run:


Afterwards, fix the following five lines:

scenarios/cinema_demo.xml: line 106
scenarios/triggers_demo.xml: line 69
scenarios/units_demo.xml: line 44
skirmishes/gallic_fields_(3).xml: line 98
tutorials/introductory_tutorial.xml: line 88

[Update]: there appears to be consensus to perform the following renames:

  • replace spaces with underscores
  • replace uppercase with lowercase
  • remove hyphens
  • remove parentheses
  • remove player numbers

Still to be decided:

  • what to do with the scenarios/Height Map *.png files
  • what to do with the three Azure Coast scenarios (identical maps, different player numbers)
  • what to do with the two Flight_demo scenarios (different maps, similar function)
  • what to do with the two Sandbox - Ptolemies scenarios (different maps, similar function; all other factions have only one sandbox)
  • how to differentiate similary named skirmishes, e.g. Corinthian Isthmus (different maps, similar names)
Test Plan

Check if everything works after renaming.

Diff Detail

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

Event Timeline

Nescio created this revision.Nov 15 2017, 1:34 PM
Owners added a subscriber: Restricted Owners Package.Nov 15 2017, 1:34 PM
Vulcan added a subscriber: Vulcan.Nov 15 2017, 1:41 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content

mv, svn add, svn rm? Pretty sure that messes up the revision history. Just use svn mv then it will count as the same file when doing svn log or svn blame.
Also without using a loop, the script can't really be reviewed.
I'd love to get rid of these numbers, but I realize they should be there in order to distinguish the same mapname for different playercounts.

If we're already renaming everything, we should also rename some odd mapnames to be consistent, mostly the capitalization like temperate_map.
There are only 2 skirmish maps with 2v2 and 3v3 in their name, but I guess that can remain as at least the latter is specially crafted for that team configuration.
Laconia 01 - not sure what we need that 01 for.
scenario maps dont have an underscore before the number while skirmish maps do.

The script would first have to rename the exceptions and then replace the spaces with underscores in each occurrence.
Could also do it in python or whatever if you prefer that (bash has very counterintuitive syntax often).
I don't want to look at a patch explicitly renaming these files for the reason you stated and since I'm never sure whether rename patches apply and end up renaming manually.

Nescio added a comment.EditedNov 15 2017, 3:02 PM
In D1042#40908, @elexis wrote:

mv, svn add, svn rm? Pretty sure that messes up the revision history. Just use svn mv then it will count as the same file when doing svn log or svn blame.

Everytime I use svn mv phabricator (or actually arc) starts complaining, hence the roundabout mv, svn add, svn rm.

In D1042#40908, @elexis wrote:

The script would first have to rename the exceptions and then replace the spaces with underscores in each occurrence.
Could also do it in python or whatever if you prefer that (bash has very counterintuitive syntax often).
I don't want to look at a patch explicitly renaming these files for the reason you stated and since I'm never sure whether rename patches apply and end up renaming manually.

A loop with rules and exceptions would be more efficient. However, the basic shell script I uploaded, which renames the files one by one, has the advantage that it can easily be followed, as if you manually renamed the files individually via the command line.

In D1042#40910, @Nescio wrote:
In D1042#40908, @elexis wrote:

mv, svn add, svn rm? Pretty sure that messes up the revision history. Just use svn mv then it will count as the same file when doing svn log or svn blame.

Everytime I use svn mv phabricator (or actually arc) starts complaining, hence the roundabout mv, svn add, svn rm.

Sure, but we don't want to use Phabricator in this case to begin with and committing files to svn must still use svn mv to keep the revision history.

A loop with rules and exceptions would be more efficient. However, the basic shell script I uploaded, which renames the files one by one, has the advantage that it can easily be followed, as if you manually renamed the files individually via the command line.

Yes but I also have to check every rename manually instead of the script to ensure that there isn't any typo.
Doing the svn mv myself would be the fastest option otherwise.
I can look into it after my next commit.

In D1042#40908, @elexis wrote:

If we're already renaming everything, we should also rename some odd mapnames to be consistent, mostly the capitalization like temperate_map.
There are only 2 skirmish maps with 2v2 and 3v3 in their name, but I guess that can remain as at least the latter is specially crafted for that team configuration.
Laconia 01 - not sure what we need that 01 for.
scenario maps dont have an underscore before the number while skirmish maps do.

Here you go:


In D1042#40911, @elexis wrote:

Yes but I also have to check every rename manually instead of the script to ensure that there isn't any typo.

Just run the script in your local repository and check the output produced in the terminal. That's what I did to remove the typos.

By the way, it also renames those six png images in the scenarios/ folder; I have no idea what their purpose is, though.

All lowercase filenames comes unexpected. Would be consistent with all other files though.
@leper agrees with the change?

These png files are heightmaps that can be imported in atlas, see https://trac.wildfiregames.com/wiki/Atlas_Manual_Heightmap_Import and palaxins guide linked there, see also d957.

The images seem a bit out of place, but not sure what other place to put them in (maybe a subdirectory?)

In D1042#40914, @elexis wrote:

All lowercase filenames comes unexpected. Would be consistent with all other files though.

Given that the title is just about replacing " " with "_" that seems unfortunate.

@leper agrees with the change?

Why wouldn't I? (I assume my tools can handle with simple file moves, if they cannot I'll be able to work around that with a tiny amount of sed.)

About the script itself, why not use find, tr, and then svn mv? This should be a one-liner.

Nescio added a comment.EditedNov 15 2017, 8:38 PM
In D1042#40914, @elexis wrote:

All lowercase filenames comes unexpected. Would be consistent with all other files though.
@leper agrees with the change?

Oh, I thought you asked for it (“temperate_map”); anyway, I agree no unnecessary capitalization looks better.

In D1042#40914, @elexis wrote:

The images seem a bit out of place, but not sure what other place to put them in (maybe a subdirectory?)

I was just wondering what they are for (because there is not always a file name match between a png and a xml); I don't have any objections to them being there.

Nescio retitled this revision from rename maps/ file names to use underscores (instead of spaces) to rename maps/ file names to use underscores (instead of spaces and capitalization).Nov 15 2017, 8:39 PM

Ah, also this will not prevent any further files with that format from being added.

temperate map is already present in the current code, didn't intend to ask for it. But lowercase is acceptable to me while at it.

Thanks for the response leper! (I just didn't want to push such a change without a second opinion)

Nescio can you try that oneliner (or more lines if there are exceptions)?

In D1042#40963, @elexis wrote:

Nescio can you try that oneliner (or more lines if there are exceptions)?

Which one?

If you upload a script that changes the capitalization and replaces space with underscore that would be quicker to confirm for me.

An exception that requires a a manual svn mv prior to the script would be Azure Coast(4) (missing space) and removing the 01 from Laconia.

Also take care about the JS files which are referenced in the XML file and need a manual change. You can just tell me the filenames to update (or I can just check the few maps with JS files I guess)

Nescio added a comment.EditedNov 16 2017, 4:06 PM
In D1042#41032, @elexis wrote:

Also take care about the JS files which are referenced in the XML file and need a manual change. You can just tell me the filenames to update (or I can just check the few maps with JS files I guess)

Good point! Here they are:
scenarios/cinema_demo.xml: line 106
scenarios/triggers_demo.xml: line 69
scenarios/units_demo.xml: line 44
skirmishes/gallic_field_(3).xml: line 98
(The other .js files were not renamed, because they already have lower case and underscores.)

In D1042#41032, @elexis wrote:

If you upload a script that changes the capitalization and replaces space with underscore that would be quicker to confirm for me.

The _2.sh files I uploaded yesterday do exactly that. I've already run them a few times to spot and correct all typos. I suppose there must be many other ways to do the same file moved in a much shorter script, but my programming skills are rather limited, and the longer but straightforward .sh files I uploaded work.

By the way, should the ten scripts in maps/scripts/ also be renamed?

Nescio updated this revision to Diff 4214.Nov 16 2017, 5:30 PM
Nescio edited the summary of this revision. (Show Details)

Also renamed introductory tutorial.

Nescio edited the summary of this revision. (Show Details)Nov 16 2017, 5:32 PM

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content
In D1042#41032, @elexis wrote:

If you upload a script that changes the capitalization and replaces space with underscore that would be quicker to confirm for me.

The _2.sh files I uploaded yesterday do exactly that. I've already run them a few times to spot and correct all typos. I suppose there must be many other ways to do the same file moved in a much shorter script, but my programming skills are rather limited, and the longer but straightforward .sh files I uploaded work.

That's why I mentioned all the needed (well, except for xargs in case you want to be sure) tools above. As always man toolname does provide useful information.

To provide and example

$ echo "foo bar baz BarBAZ" | tr " A-Z" "_a-z"
foo_bar_baz_barbaz

The benefit of that is that the amount of things to review is reduced tremendously.

Itms added a subscriber: Itms.Feb 28 2018, 11:41 AM

I took a look at this, P111 would detect bad filenames in the future, I suggest testing that bear by pasting the section into .coafile after modifying the maps.

The following command, which is what leper was talking about, makes things easy (including reviewing) but

  • Don't run it on scripts or rmgen stuff
  • It doesn't fix the references to trigger scripts. Maybe the diff should be those in-file changes, along with a single shell script that runs the command on the correct directories
find folder -type f -exec sh -c "echo '{}' | tr -d '-' | tr ' A-Z' '_a-z' | tr -s '_' | xargs svn mv '{}'" ";"

(I found that _-_ is ugly, as a result the command is ugly as well)

Nescio updated this revision to Diff 5972.Feb 28 2018, 2:46 PM

Updated: this patch now corrects script references; files can be renamed via Itms' command

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

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

Nescio added a comment.EditedFeb 28 2018, 2:52 PM

Thanks, Itms! To summarize, I think the correct order now is:

  • apply this diff first
  • leave maps/random/ as is (already has consistent naming with underscores)
  • leave maps/scripts/ as is (no spaces or hyphens there)
  • rename files in maps/tutorials/ with Itms' command
  • rename files in maps/skirmishes/ with Itms' command
  • rename files in maps/scenarios/ with Itms' command and subsequently correct some renamed file names there:
svn mv 'azure_coast(2).pmp' 'azure_coast_(2).pmp'
svn mv 'azure_coast(2).xml' 'azure_coast_(2).xml'
svn mv 'azure_coast(4).pmp' 'azure_coast_(4).pmp'
svn mv 'azure_coast(4).xml' 'azure_coast_(4).xml'
svn mv 'laconia_01.pmp' 'laconia.pmp'
svn mv 'laconia_01.xml' 'laconia.xml'
svn mv 'sandbox_mauryans.pmp' 'sandbox_mauryas.pmp'
svn mv 'sandbox_mauryans.xml' 'sandbox_mauryas.xml'
Nescio edited the summary of this revision. (Show Details)Feb 28 2018, 2:54 PM
Nescio added a comment.EditedFeb 28 2018, 2:59 PM

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

Or instead, run:


and manually correct afterwards:

  • scenarios/cinema_demo.xml: line 106
  • scenarios/triggers_demo.xml: line 69
  • scenarios/units_demo.xml: line 44
  • skirmishes/gallic_fields_(3).xml: line 98
  • tutorials/introductory_tutorial.xml: line 88
Nescio edited the summary of this revision. (Show Details)Feb 28 2018, 3:00 PM
vladislavbelov added inline comments.
binaries/data/mods/public/maps/skirmishes/Gallic Fields (3).xml
98

We need to backslash braces ((, )) on Linux and macOS too. May be we need to remove it too? @Itms

Nescio edited the summary of this revision. (Show Details)Feb 28 2018, 3:26 PM
Itms added inline comments.Mar 3 2018, 4:50 PM
binaries/data/mods/public/maps/skirmishes/Gallic Fields (3).xml
98

Braces are not nice either, I agree. But I'm not sure what would be a good naming convention for the number of players on skirmishes. Maybe gallic_fields_3p.xml?

Itms added inline comments.Mar 11 2018, 7:11 PM
binaries/data/mods/public/maps/skirmishes/Gallic Fields (3).xml
98

Got another idea, we could write gallic_fields-3.xml.

Currently all skirmish files include the number of players within parentheses in the file name, e.g. Acropolis Bay (2).xml Is this actually necessary? Most maps have just one version.
If those skirmish player numbers were removed, file names would be shorter and simultaneously more consistent with other maps, which don't include player numbers in their name either. There are just two cases to distinguish different versions of the same map:

scenarios/Azure Coast.xml
scenarios/Azure Coast(2).xml
scenarios/Azure Coast(4).xml
skirmishes/Corinthian Isthmus (2).xml
skirmishes/Corinthian Isthmus (4).xml
binaries/data/mods/public/maps/skirmishes/Gallic Fields (3).xml
98

Although I agree with removing parentheses and other special characters, I'm not sure replacing (3) with -3, _3, _3p, _3x, or something similar would be an improvement.

It might be better to omit the player number altogether.

Itms added a comment.Mar 12 2018, 2:42 PM
In D1042#56753, @Nescio wrote:

Currently all skirmish files include the number of players within parentheses in the file name, e.g. Acropolis Bay (2).xml Is this actually necessary? Most maps have just one version.

Since we do have maps with the issue, we still need to find something, so we might as well use that for all maps (the aim was to improve consistency after all).

Yes, the aim of this patch is to improve map files naming consistency and to remove inconvenient white spaces. However, random maps and scenarios do not include player numbers in the file name, only skirmishes do; furthermore, other data, such as map size, aren't included in the file name either. Removing the player numbers from the file names would thus improve naming consistency and also allow the removal of parentheses ().
Nevertheless, we still need to find a proper way to differentiate the "Azure Coast" scenarios and the "Corinthian Isthmus" skirmishes. Any suggestions, anyone?

Nescio added a comment.EditedJan 3 2019, 10:37 AM

Spaces and special characters in file names are still problematic, so it would be nice if we could simply use snake case for all map files.

To recapitulize, I believe the script:

  • replaces spaces with underscores
  • replaces hyphens with underscores
  • removes underscores when followed by an underscore
  • replaces capitals with lower case

Not yet decided:

  • what to do with _(2) and similar

Not yet decided:
what to do with _(2) and similar

It sounds easier to remove the teamdescriptions such (2) and (3v3), remove one of the map variants if there are two variants with identical pmp file (#5361), rename legitimate variants to have the difference not be reflected by the number of teams but an artistic title.
But perhaps there was a reason I didn't notice for the teamnumbers being part of the filename.

camel case

You mean lower case?

Oops, I meant snake case (azure_coast instead of Azure Coast or AzureCoast).

Nescio added a comment.EditedJan 5 2019, 9:00 PM

There are currently several sets of files which are distinguished by a number in their file name:

  • "scenarios/Azure Coast", " Coast(2)", and "Coast(4)"
  • "scenarios/Flight_demo" and "Flight_demo_2"
  • "scenarios/Sandbox - Ptolemies" and " Ptolemies 2"
  • "skirmishes/Corinthian Isthmus (2)" and " (4)"
  • "skirmishes/Cycladic Archipelago (2)" and " (3)"
  • "skirmishes/Greek Acropolis (2)" and " (4)"
  • "skirmishes/Median Oasis (2)" and " (4)"
  • "skirmishes/Neareastern Badlands (2)" and " (4)"

However, I don't know if there are more differences than just the number of players. How does one check?

Also, what to do with the "scenarios/Height Map *.png" files?

Stan added a subscriber: Stan.Jan 5 2019, 9:08 PM
In D1042#69408, @Nescio wrote:

However, I don't know if there are more differences than just the number of players. How does one check?
Also, what to do with the "scenarios/Height Map *.png" files?

Well you could open the maps in Atlas. But I believe that's the main difference.

Nescio added a comment.Jan 5 2019, 9:55 PM

Well you could open the maps in Atlas.

The Azure Coast scenarios seem to use the same map, so perhaps it's unnecessary to keep three versions.

The skirmishers are all different; the (4) versions are larger than the (2) versions, have a different layout (e.g. river vs no water) etc. So perhaps we should rename the (2)s to "small" and the (4)s to "large" if we want to get rid of the numbers.

Nescio edited the summary of this revision. (Show Details)Sat, Jan 26, 1:11 PM

Still to be decided:
1 what to do with the scenarios/Height Map *.png files
2 what to do with the three Azure Coast scenarios (identical maps, different player numbers)
3 what to do with the two Flight_demo scenarios (different maps, similar function)
4 what to do with the two Sandbox - Ptolemies scenarios (different maps, similar function; all other factions have only one sandbox)
5 how to differentiate similary named skirmishes, e.g. Corinthian Isthmus (different maps, similar names)

How about the following?

  • 2, 3, 4: keep only one version, delete the others
  • 5: rename them to _small and _large
Itms added a comment.Thu, Feb 7, 10:50 PM
In D1042#71075, @Nescio wrote:

Still to be decided:
1 what to do with the scenarios/Height Map *.png files
2 what to do with the three Azure Coast scenarios (identical maps, different player numbers)
3 what to do with the two Flight_demo scenarios (different maps, similar function)
4 what to do with the two Sandbox - Ptolemies scenarios (different maps, similar function; all other factions have only one sandbox)
5 how to differentiate similary named skirmishes, e.g. Corinthian Isthmus (different maps, similar names)

How about the following?

  • 2, 3, 4: keep only one version, delete the others
  • 5: rename them to _small and _large

I think 2) should be kept: it's nice, for skirmishes, to be able to choose depending on the number of players, instead of saying that Azure Coast is only for two players.

For 3 and 4 there should be merges indeed, but let's make sure we keep all the interesting things from both merged maps each time.

For 5, I would rather find a unique and appealing name allowing to discriminate the maps easily. Small/Large breaks immersion quite a bit.

elexis added a comment.Fri, Feb 8, 8:24 PM

I think 2) should be kept: it's nice, for skirmishes, to be able to choose depending on the number of players, instead of saying that Azure Coast is only for two players.

The problem with that is that this
(a) it means it needs a copy of the terrain file for each instance (we have the file three times). For Azure Coast this case it's only 1.5mb, but other maps use 10mb
(b) if we already have the copy, why not add so much art to it, that the maps become an actually unique experience. For Azure Coast, one can say that the map terrain is so amazing and unique that one wants to play it in multiple scenarios. Notice that the entities are really unique (players start at the shore with a dock and a tower).
(c) if the argument is true in general, then it would apply to the other maps as well. So then we would be inviting to copy around many maps and just differ the playercount. But we don't want so many copied files, and we don't want to get the same map multiple times when we browse through the mapselection list (but rather get a map variant choice after having selected a map, if it's essentially the same map). Also there are random maps that can load *pmp files since a23, so I would encourage future devs to implement such map variants using that.
I'd be ok with it being a exemption though.

Probably agree with 3-5 although I didn't look at how most maps look. The first flight demo map looks bad and doesn't seem to have anything that the second one doesn't have.

For 5, I would rather find a unique and appealing name allowing to discriminate the maps easily. Small/Large breaks immersion quite a bit.

Well, I'm open for suggestions, of course. *_large and *_small are probably less problematic placeholders than * (2) and * (4). Also, this patch is about renaming the file names. The map names as displayed in game can be different and don't have to be changed.

1 what to do with the scenarios/Height Map *.png files