Unified human FG name files

[P-D-E]

Thank you for submitting a pull request and becoming a contributor to Vega Strike: Upon the Coldest Sea.

Please answer the following:

Code Changes:

• [x] Have the PR Validation Tests been run? See https://github.com/vegastrike/Vega-Strike-Engine-Source/wiki/Pull-Request-Validation
• [ ] This is a documentation change only

Issues:

Purpose:

• What is this pull request trying to do? Replace duplicate flight group file names with a new standard one for all the human factions
• What release is this for? 0.8.x
• Is there a project or milestone we should apply this to? 0.8.x

[BenjamenMeyer]

@P-D-E looks good.

[stephengtuggy]

Has this been play tested yet?

[P-D-E]

@stephengtuggy I didn't follow the standard playtest and did this:

• take away the human factions' files from universe/fgnames leaving only names.txt
• add a print line to report faction and filename with relative path here

   for name in names:
       try:
           with open (name,'r') as f:
               bnl = f.readlines()
           print(faction, 'names found as', name, 'e.g.', bnl[0])  # <--- this one
           break

• play a game with at least a few human factions in the sector, checking the terminal for that print at every occurence; names.txt has been used for the factions without a corresponding file.

[stephengtuggy]

@P-D-E So you did some testing specific to this change set. Great.

I think we will still want to run the standard set of tests from https://github.com/vegastrike/Vega-Strike-Engine-Source/wiki/Pull-Request-Validation before merging this. Also, did you check to make sure that, e.g. Rlaan factions still work properly?

[P-D-E]

@P-D-E So you did some testing specific to this change set. Great. did you check to make sure that, e.g. Rlaan factions still work properly?

Of course, that was the point of the test; names.txt has been used for the factions without a corresponding file, not for every faction in game.

[P-D-E]

@BenjamenMeyer @stephengtuggy Standard play test passed wrt this particular issue, but two more emerged, one related and one not:

1. Sub-factions (citizen, merchant marine) have no specific files, so they default to names.txt instead of their species. Should a fallback logic be implemented?
2. Introduction, keep scrolling down until all text disappears:

Traceback (most recent call last):
  File "<string>", line 7, in <module>
  File "/opt/games/vs-0.5.3/Vega-Strike-Engine-Source/data/modules/GUI.py", line 434, in redrawIfNeeded
    self.rooms[i].redrawIfNeeded()
  File "/opt/games/vs-0.5.3/Vega-Strike-Engine-Source/data/modules/GUI.py", line 663, in redrawIfNeeded
    self.redraw()
  File "/opt/games/vs-0.5.3/Vega-Strike-Engine-Source/data/modules/GUI.py", line 669, in redraw
    GUIRootSingleton.broadcastRoomMessage(self.getIndex(),'draw',None)
  File "/opt/games/vs-0.5.3/Vega-Strike-Engine-Source/data/modules/GUI.py", line 397, in broadcastRoomMessage
    self.objects[i][1].onMessage(message,params)
  File "/opt/games/vs-0.5.3/Vega-Strike-Engine-Source/data/modules/GUI.py", line 775, in onMessage
    self.onDraw(params)
  File "/opt/games/vs-0.5.3/Vega-Strike-Engine-Source/data/modules/GUI.py", line 796, in onDraw
    self.draw()
  File "/opt/games/vs-0.5.3/Vega-Strike-Engine-Source/data/modules/GUI.py", line 1633, in draw
    self._updateListItemText()
  File "/opt/games/vs-0.5.3/Vega-Strike-Engine-Source/data/modules/GUI.py", line 1624, in _updateListItemText
    txt = self._visItemText(i)
  File "/opt/games/vs-0.5.3/Vega-Strike-Engine-Source/data/modules/GUI.py", line 1617, in _visItemText
    txt = str(self.items[i+self.firstVisible])
TypeError: list indices must be integers or slices, not float

The gui becomes unresponsive, the Continue button does nothing. (Ignore the 0.5.3 in the folder name, it's the one I use with git since then, I had checked out 0.8.x for the test)

Should I file an issue for the latter?

[stephengtuggy]

@P-D-E Yes, I think we'll need to address that first issue with a fallback mechanism, before this PR gets merged.

As for the second issue, I've already filed a ticket for that here: https://github.com/vegastrike/Assets-Masters/issues/30

[P-D-E]

@P-D-E Yes, I think we'll need to address that first issue with a fallback mechanism, before this PR gets merged.

This does the job, is it acceptable?

    base_faction = faction.replace('_citizen', '').replace('_guild', '').replace('_briin', '').replace('n_merchant_marine', '')
    base_filename = os.path.join('universe', 'fgnames', base_faction + '.txt')
    names = [
             filename,
             base_filename,
             os.path.join('../', filename),
             os.path.join('../', base_filename),
             os.path.join('universe', 'fgnames', 'names.txt'),
             os.path.join('..', 'universe', 'names.txt'),
             os.path.join('universe', 'names.txt')
            ]

As for the second issue, I've already filed a ticket for that here: vegastrike/Assets-Masters#30

Good!

[stephengtuggy]

@P-D-E Yes, I think we'll need to address that first issue with a fallback mechanism, before this PR gets merged.

This does the job, is it acceptable?

    base_faction = faction.replace('_citizen', '').replace('_guild', '').replace('_briin', '').replace('n_merchant_marine', '')
    base_filename = os.path.join('universe', 'fgnames', base_faction + '.txt')
    names = [
             filename,
             base_filename,
             os.path.join('../', filename),
             os.path.join('../', base_filename),
             os.path.join('universe', 'fgnames', 'names.txt'),
             os.path.join('..', 'universe', 'names.txt'),
             os.path.join('universe', 'names.txt')
            ]

Looks good to me. @Loki1950 , can you take a look at the above code snippet?

[Loki1950]

Looks good to me.

[P-D-E]

For the record, here are a few debugging prints showing the fallback in action:

merchant_guild names found as universe/fgnames/merchant.txt e.g. Aachen
rlaan_briin names found as universe/fgnames/rlaan.txt e.g. Abelia_x_grandiflora
aeran_merchant_marine names found as universe/fgnames/aera.txt e.g. abelsonite
rlaan_citizen names found as universe/fgnames/rlaan.txt e.g. Abelia_x_grandiflora
merchant_guild_citizen names found as universe/fgnames/merchant.txt e.g. Aachen

[stephengtuggy]

@P-D-E if it's good enough for @Loki1950 , it's good enough for me. Why don't you go ahead and commit that change, and then I think we'll be ready to merge this PR.

[stephengtuggy]

(I was mainly concerned about whether the rlaan and rlaan_briin should use the same set of faction names or not)

[stephengtuggy]

On second thought: I'm getting a crash on Windows. Let me investigate a little further. We might have an OS-specific problem here.

[stephengtuggy]

On second thought: I'm getting a crash on Windows. Let me investigate a little further. We might have an OS-specific problem here.

I'm getting the following on my Windows machine:

Loading active missions True
void Mission::DirectorLoop(): Python error occurred
Traceback (most recent call last):
  File "C:\devel\Assets-Production\modules\missions\privateer.py", line 34, in Execute
    i.Execute()
  File "C:\devel\Assets-Production\modules\random_encounters.py", line 290, in Execute
    generate_dyn_universe.KeepUniverseGenerated()
  File "C:\devel\Assets-Production\modules\generate_dyn_universe.py", line 312, in KeepUniverseGenerated
    ReloadUniverse()
  File "C:\devel\Assets-Production\modules\generate_dyn_universe.py", line 277, in ReloadUniverse
    GenerateAllShips() ###Insert number of flight groups and max ships per fg
  File "C:\devel\Assets-Production\modules\generate_dyn_universe.py", line 80, in GenerateAllShips
    fgnames.append(fg_util.GetRandomFGNames(-1,faction_ships.factions[fnr]))
  File "C:\devel\Assets-Production\modules\fg_util.py", line 101, in GetRandomFGNames
    flightgroupnamelist[faction]=ReadBaseNameList(faction)
  File "C:\devel\Assets-Production\modules\fg_util.py", line 74, in ReadBaseNameList
    filename = os.path.join('universe', 'fgnames', faction + '.txt')
NameError: name 'os' is not defined

Do we maybe just need an import os line near the top?

[P-D-E]

(I was mainly concerned about whether the rlaan and rlaan_briin should use the same set of faction names or not)

The '_briin' substitution can be taken away as soon as the rlaan_briin.txt is provided; meanwhile I think it makes more sense to fall back to the main species file.

Do we maybe just need an import os line near the top?

Oops, forgot to commit that one! Done.

[stephengtuggy]

@P-D-E Yes, I think we'll need to address that first issue with a fallback mechanism, before this PR gets merged.

This does the job, is it acceptable?

    base_faction = faction.replace('_citizen', '').replace('_guild', '').replace('_briin', '').replace('n_merchant_marine', '')
    base_filename = os.path.join('universe', 'fgnames', base_faction + '.txt')
    names = [
             filename,
             base_filename,
             os.path.join('../', filename),
             os.path.join('../', base_filename),
             os.path.join('universe', 'fgnames', 'names.txt'),
             os.path.join('..', 'universe', 'names.txt'),
             os.path.join('universe', 'names.txt')
            ]

Looks good to me. @Loki1950 , can you take a look at the above code snippet?

@P-D-E Once you commit the above change, I think this will be ready to merge. Pending a brief play test or two.

[P-D-E]

@P-D-E Once you commit the above change, I think this will be ready to merge. Pending a brief play test or two. @stephengtuggy Standard play test passed again, on Linux (https://github.com/vegastrike/Assets-Production/pull/74 included); feel free to test on the other platforms.

[stephengtuggy]

@P-D-E Cool.

The only thing is, I don't see the following line in this PR yet:

base_faction = faction.replace('_citizen', '').replace('_guild', '').replace('_briin', '').replace('n_merchant_marine', '')

[P-D-E]

@stephengtuggy Done