Unified human FG name files #63

P-D-E · 2021-06-07T14:15:27Z

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

Please answer the following:

Code Changes:

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

modules/fg_util.py

BenjamenMeyer · 2021-06-08T04:34:27Z

@P-D-E looks good.

stephengtuggy

LGTM!

stephengtuggy · 2021-09-02T20:20:20Z

Has this been play tested yet?

P-D-E · 2021-09-02T20:50:13Z

@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 · 2021-09-02T21:10:59Z

@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 · 2021-09-02T21:42:32Z

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

BenjamenMeyer

LGTM pending a standard play test

P-D-E · 2021-09-03T15:24:09Z

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

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?
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 · 2021-09-03T16:52:48Z

@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: vegastrike/Assets-Masters#30

P-D-E · 2021-09-03T17:34:44Z

@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 · 2021-09-03T20:07:35Z

@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 · 2021-09-03T22:35:31Z

Looks good to me.

P-D-E · 2021-09-03T22:42:32Z

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 · 2021-09-04T00:27:22Z

@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 · 2021-09-04T00:27:58Z

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

stephengtuggy · 2021-09-04T01:22:30Z

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 · 2021-09-04T01:57:25Z

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 · 2021-09-04T08:27:11Z

(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 · 2021-09-04T13:39:44Z

@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 · 2021-09-04T14:31:57Z

@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 (#74 included); feel free to test on the other platforms.

stephengtuggy · 2021-09-04T14:36:41Z

@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 · 2021-09-04T14:45:28Z

@stephengtuggy Done

stephengtuggy

Cool! There we go.

modules/fg_util.py

Utf-8 encoding enforced on file read to prevent Windows from defaulting to cp1252

Unified human FG name files

dca03a3

P-D-E added this to the 0.8.x milestone Jun 7, 2021

P-D-E requested review from BenjamenMeyer, royfalk, evertvorster, stephengtuggy and Loki1950 June 7, 2021 14:15

P-D-E mentioned this pull request Jun 7, 2021

Utf8 fgnames #62

Merged

2 tasks

Loki1950 approved these changes Jun 7, 2021

View reviewed changes

BenjamenMeyer reviewed Jun 8, 2021

View reviewed changes

modules/fg_util.py Outdated Show resolved Hide resolved

Reformatted names list

0dbc059

P-D-E marked this pull request as ready for review September 2, 2021 16:16

stephengtuggy approved these changes Sep 2, 2021

View reviewed changes

BenjamenMeyer approved these changes Sep 2, 2021

View reviewed changes

Added missing import os

dc6afa5

Added fallback to base race names

e7d1d80

stephengtuggy approved these changes Sep 4, 2021

View reviewed changes

stephengtuggy reviewed Sep 4, 2021

View reviewed changes

modules/fg_util.py Outdated Show resolved Hide resolved

stephengtuggy self-requested a review September 4, 2021 15:41

Utf-8 encoding enforced on file read

4338f32

Utf-8 encoding enforced on file read to prevent Windows from defaulting to cp1252

stephengtuggy merged commit 77b105e into vegastrike:master Sep 4, 2021

P-D-E deleted the human_fgnames_dedup branch September 29, 2021 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unified human FG name files #63

Unified human FG name files #63

P-D-E commented Jun 7, 2021 •

edited

Loading

BenjamenMeyer commented Jun 8, 2021

stephengtuggy left a comment

stephengtuggy commented Sep 2, 2021

P-D-E commented Sep 2, 2021

stephengtuggy commented Sep 2, 2021

P-D-E commented Sep 2, 2021

BenjamenMeyer left a comment

P-D-E commented Sep 3, 2021

stephengtuggy commented Sep 3, 2021 •

edited

Loading

P-D-E commented Sep 3, 2021

stephengtuggy commented Sep 3, 2021

Loki1950 commented Sep 3, 2021

P-D-E commented Sep 3, 2021

stephengtuggy commented Sep 4, 2021

stephengtuggy commented Sep 4, 2021

stephengtuggy commented Sep 4, 2021

stephengtuggy commented Sep 4, 2021

P-D-E commented Sep 4, 2021

stephengtuggy commented Sep 4, 2021 •

edited

Loading

P-D-E commented Sep 4, 2021

stephengtuggy commented Sep 4, 2021

P-D-E commented Sep 4, 2021

stephengtuggy left a comment

Unified human FG name files #63

Unified human FG name files #63

Conversation

P-D-E commented Jun 7, 2021 • edited Loading

Issues:

BenjamenMeyer commented Jun 8, 2021

stephengtuggy left a comment

Choose a reason for hiding this comment

stephengtuggy commented Sep 2, 2021

P-D-E commented Sep 2, 2021

stephengtuggy commented Sep 2, 2021

P-D-E commented Sep 2, 2021

BenjamenMeyer left a comment

Choose a reason for hiding this comment

P-D-E commented Sep 3, 2021

stephengtuggy commented Sep 3, 2021 • edited Loading

P-D-E commented Sep 3, 2021

stephengtuggy commented Sep 3, 2021

Loki1950 commented Sep 3, 2021

P-D-E commented Sep 3, 2021

stephengtuggy commented Sep 4, 2021

stephengtuggy commented Sep 4, 2021

stephengtuggy commented Sep 4, 2021

stephengtuggy commented Sep 4, 2021

P-D-E commented Sep 4, 2021

stephengtuggy commented Sep 4, 2021 • edited Loading

P-D-E commented Sep 4, 2021

stephengtuggy commented Sep 4, 2021

P-D-E commented Sep 4, 2021

stephengtuggy left a comment

Choose a reason for hiding this comment

P-D-E commented Jun 7, 2021 •

edited

Loading

stephengtuggy commented Sep 3, 2021 •

edited

Loading

stephengtuggy commented Sep 4, 2021 •

edited

Loading