Patcher modules layout

[Utumno]

@lojack5 said:

bash\
    game\
        skyrim\
            __init__.py  # brings in all the files
            patcher.py  # defines/modifies the base patchers
            records.py  # defines the records used by skyrim
            ...etc...
    patcher\
        __init__.py # brings in all the files
        ...etc...

Since I am about to actually move the patchers out I want us to discuss the layout in detail - that is in class level. Chime in!

Edit: I mean to clarify, among others:

• how much is game independent in the patchers - in other words what is the patchers API
• which classes need to go with them (I already ripped the utilitties classes - but had to import them in basher too)
• how are the pacthers themselves going to be split in files. Of course not a file per class a la Java (yak) but are we going to split them in correspondence to the UI ? To the scan orders (notice patchers 10,20,30,40 in bosh) ?

This is of course under #3

[Utumno]

Discuss in code - last 3 commits ([layout]):

https://github.com/wrye-bash/wrye-bash/commits/utumno-refactoring-master

Questions:

• utilities.py ? Better name. Better docs in __init__.py - could use your help
• should this be oblivion/patcher/utilities.py, oblivion/patcher/patchers/XXXPatcher.py ? I think that these "utilities" are not for the patcher only
• Am I right to think that the classes I put in the bash/patcher/base.py module are game independent and actually comprise the patcher API ? What other classes should go in there ?
• the RecordsGroups.py are indeed oblivion specific ? Should we extract an API ? What should the module name be (lowercase) ?

[Sharlikran]

I looked at the utilities.py and RecordGroups.py and I don't think they are Oblivion specific. Those would probably have to be updated for Skyrim and FO3/FNV at some point.

I don't know what to call the file either. They are basically helpers for the records like CELL, WRLD, DIAL. I'm not sure all of them are callback routines either nor would you want to be that strict. They are not really common routines and even then, common routines like MreHasEffects will be different for each game mode.

I say leave it the name it is and just specify in a comment at the top what the file is for.

[Sharlikran]

I might be able to shed some light on the utilities.py file but it won't really answer your question. While working on Wrye Mash I discovered there is a "Utilities" tab where things like Mlox show up. Which was later moved to the bottom bar where the icons show up now for BOSS and such.

Those could be moved to another file specific for the game mode so that they can be updated as needed to suit the needs of that game and how Wrye reads and writes to the Bash Patch to fit that game's plugin format.

While you are doing this please look for the byte definition for the header and record sizes.

FO3/FNV/TES4/TES5 : [4 byte Signature] [ 2 byte size ]

ESCE [ 10 00 ] - 2 Bytes for the size

Morrowind : [4 byte Signature] [ 4 byte size ]

ESCE [ 10 00 00 00 ] - 4 Bytes for the size

Please refactor the record and header size so that it becomes game specific.

[Sharlikran]

@lojack5 in regards to my previous request, would class RecordHeader(brec.BaseRecordHeader): already handle that if changed correctly? Or would that be handled in ModReader?

[lojack5]

No, you'd have to update the subrecord unpacking code. RecordHeader only handles the record headers. Relevant section here - you'd want to generalize it so it can be derived and implemented per-game.

[Utumno]

I resumed work on this finally on my https://github.com/wrye-bash/wrye-bash/compare/utumno-refactoring-master branch - need feedback - see there

Among others - shouldn't we move the record classes out of skyrim/oblivion.py ?

Another idea - a new module bash/records with submodules for each game and base record classes on top level - I actually like this - what you say ?

[Sharlikran]

I am fine with whatever you feel is appropriate.

        for recClass in (MreRecord.type_class[x] for x in ('NPC_',)):
            self.recAttrs_class[recClass] = {
                u'Actors.AIData': ('aggression','confidence','energyLevel','responsibility','services','trainSkill','trainLevel'),

One thing I am going to do in the next few days on a test branch I have is move the patcher routines to skyrim.py, oblivion.py, fallout3.py, falloutnv.py. This way I can make side by side comparasons of the constants used for the patchers as shown in the above code. It eliminates the need for 4 if/fsName statements by just saying self.recAttrs_class[recClass] = bosh.game.thePatcherSetName. It would be one decleration and connect to the set for each game mode.

So if you want to move those to another file that's fine also. Whatever you do I'll just follow suit.

[Utumno]

Stay tuned - I already started the rip - I disagree with patchers routines in gamename.py files - we aim at 2000-3000 lines at most per file (excluding files with constants) - not 20.000.

See the commits at my branch - started moving the patcher routines out of bosh and into patcher/oblivion/

Similar structure for other games

But I also believe we should go for records/gamename modules

[lojack5]

My personal opinion on how to divvy up the game specific code:

bush/game/<game_name> has every bit of specific code for games (further divided into specialized sub-packages). Then if you want to split up stuff into bash/records, have the __init__.py part of that module pull in the applicable information form bush/game/<game_name>.

Reasoning: makes maintenance of the files easier, makes it easier to understand the code for those unfamiliar with it, makes it easier to add new games. Because all the game information is in one place, rather than piece-meal scattered around.

Obviously not the only way to do it, but my personal choice. And we don't have to have everything in oblivion.py as is the case now, for example, just change to somehting similar to this;

bash\
    game\
        oblivion\
            __init__.py
            records.py
            constants.py
            ...etc...

[Sharlikran]

bash\
    game\
        oblivion\
            __init__.py
            records.py
            constants.py
            patcher.py
            oblivion.py
            ...etc...

I like Lojack's take on the structure. The constants.py could include the code I mentioned earlier. Is that what you already had in mind Utumno, maybe something like that?

I also like his take on what __init__.py should have so there doesn't have to be declarations to things like from patcher.oblivion.utilities import ActorLevels, CBash_ActorLevels. The code would know where to pull the information because of the game mode, or fsName or whatever you chose to use.

[Utumno]

This is not really about the games - it is mainly about the patchers
Have a look at my branch - I need to know what is game independent in there:

• in the patchers (AListPatcher should be game independent but ListPatcher (as it is now) is not)
• in the records (MelBlah)
• in the "RecordGroups" (whatever this is) - see RecordGroups.py

The most important thing is to extract a patcher API - that is Abstract Base Classes.
Please have a look at patcher/base.py - is it anywhere near the mark ?

NB: the way it is now is because I do not want to mess with bush.GetGame() (or whatever)
I am certainly open to Lo's suggestion - but even then the details must be specified - in class level.

Another option I consider is

bash/
  patcher/
    base.py
    oblivion/
    skytim/
    ...
  records/
    base.py
    oblivion/
    skytim/
  game/
    game.template
    oblivion.py
    skytim.py

But I most importantly want some feedback on the patchers API (game refactoring is another issue in this tracker)

[Sharlikran]

If Lojack likes the above option, I like that format too, I approve of this messege. 👍

[Arthmoor]

As an outsider looking in, I'd prefer Lojack's structure simply because I view things based on each supported game and it seems less disorganized that way.

[Sharlikran]

From a programming standpoint for someone unfamiliar with the code Utumno's idea has merit. I actually like that idea because then I know where the patcher routines are going to be and I don't have to guess. Either way works for me I don't care as long as the patcher API is factored out. Even though this is more about the patchers and less about the games themselves, without the changes it will be hard to continue support for future games such as FO4/TES6 if they ever happen.

[lojack5]

@Utumno: I undestand you're specifically referring to the patchers part of refactoring at the moment, but it is still game related (otherwise it wouldn't need to be refactored). What I'm saying my opinion is, is the game specific code should all be in one module. The patchers should import from there.

So

bash\
    game\
        oblivion\
            __init__.py
            records.py
            patcher.py
            ...etc...
        skyrim\ 
            __init__.py
            records.py
            patcher.py
            ...etc...
patcher\
    __init__.py
    base.py
    ...etc...

And what happens is, all the code is in one location for each game, easier to find, easier to implement a new game mode. Then, the patchers module will import the applicable sections from the currently loaded game module.

So instead of using say:
bash.patcher.oblivion.<insert function here>
You just do:
bash.patcher.<insert function here>
Because the patcher module on import already imports from the correct game module. Another good side effect of this is that we never have to explicitly import from a module with a game mode name in it. For example, say each game mode has a slightly different implementation of the RacePatcher. You would just do bash.patcher.RacePatcher, because bash.patcher has already pulled in the correct one via doing a from ..game.patcher import RacePatcher. And the bush.game module has already been set up at load time to be set to the current game mode.

The biggest reason for doing it all this way: the game switching/detection only has to happen once. bush.game will always be the current game mode, and that's set up in bush at startup. Splitting game specific files out of bush.game means we now have to setup up other modules in a similar manner so they import the correct versions depending on the game mode, ie: we'd need code like this:

if bush.game.name == 'Oblivion':
    import patcher.oblivion as patcher
elif bush.game.name == 'Skyrim':
    import patcher.skyrim as patcher
elif bush.game.name == 'Fallout 3':
    import patcher.fallout3 as patcher
# etc

Keeping all game specific files in the bush.game package negates the need for that.

[Sharlikran]

With the changes from #144 will the above code work? Because bush.game.fsName replaced bush.game.name.

if bush.game.fsName == u'Oblivion':
    import patcher.oblivion as patcher
elif bush.game.fsName == u'Skyrim':
    import patcher.skyrim as patcher
elif bush.game.fsName == u'Fallout3':
    import patcher.fallout3 as patcher
elif bush.game.fsName == u'FalloutNV':
    import patcher.falloutnv as patcher
else:
    raise BoltError( [insert error message here] )

Would you use that instead? Bacause of __init__.py will fsName be available that early in initialization?

[lojack5]

That's my point though, by splitting up the way I propose, you don't need that code at all. You just need one simple line in the patcher init file

bash\patcher\__init__.py:

from ..game import patcher

file referencing oblivion.patcher, skyrim.patcher, etc:

import patcher  #automatically is oblivion.patcher, skyrim.patcher, etc as determined by the game mode

You can customize the exact logical layout of where stuff gets imported exactly, this is just a very simple example.

#144 doesn't really have much to do with this issue. That's just adding an additional variable inside the bush.game module. Ideally though, rather than doing comparisons on games, we should be adding a boolean variable, so wherever you have code like:

if bush.game.fsname == 'Oblivion':
    # do something
elif bush.game.fsname == 'Skyrim':
   # do something else
else:
   # do something else

Say for example, those are for adding a specific menu item, instead of doing bush.game.fsname comparisons, we should add a new variable bush.game.<menuItemBoolean> to each game file, or maybe an int instead if it's more than one option, then the code becomes:

if bush.game.<menuItemInt> == 1:
    # Do one thing
elif bush.game.<menuItemInt> == 2:
    # Do something else
else:
    # Do another something

The idea behind this is, the main code of Bash doesn't need to know which game it's managing. It just needs to know certain properties. It makes it more generic, and simpler to add new games. In the above example, say TES6 comes out, you'd have to find every place that has those compound if statements and and in a check for bush.game.fsname == 'TES6Name', rather than simply making the variable the appropriate value in the game file.

[lojack5]

As a side note: this does require that bush is imported and the game set before any modules that import from it are imported. That, however is already taken care of in bash.py. The only modules imported before the game mode is set are:

• bass - constants
• barg - command line parsing
• bolt - utility functions and classes, is and should be kept game generic

every other sub-module of bash are safe to use imports from bush.game in.

[Utumno]

@lojack5: makes sense indeed - I will have to look into the bush game handling code - will ask here at #65 when I start looking into it.
We need to change things anyway as now the oblivion/skyrim/etc.py will become packages instead of modules (and the patcher will be a package also ofc - no huge files anymore !)

Till then I continue the ripping - and I would like you (when you have time) to have a look at my branch cause we need to discuss what the patchers API will be.

• Is it really possible to have the same patchers for all games though (RacePatcher...) ?
• We need some base classes (have a look at patcher\base.py)

EDIT: I just saw your second post - will have a thorough look and post back

[Utumno]

Two questions : record_groups.py and utilities.py - how game specific are those (apart from CBash) ?

record_groups is also used in bosh and utilities in basher - are they even patcher specific ?

[Sharlikran]

Those should be in a common file because they are not patcher or game specific. As for how you should organize it no idea. However, Oblivion, Skyrim, FO3/FNV all use routines from both of those.

I am going to be re-installing windows so if I don't respond for a few days now you know why.

[Utumno]

Well I am reinstalling myself in another country - gotta find a house (and a proper computer)
I kept working on the layout though - I think that the last big issue lies in basher.
In commit d2d6925 as you see I move one of the Cobl patchers to the game/oblivion/patcher/special.py - this results in game specific imports in basher - which is a nono. Actually those were always there - just made explicit by the refactoring

The solution is to dynamically create the basher patcher classes (using type())- but I would appreciate some help/advice on this by @lojack5.

I would rather close #124 and #3 together - #3 is ready but the patchers are not in their final place there.

[Utumno]

Update: proposed solution for the basher game specific imports: 592b84f f73b37b 6f56335

Promoting this to a goal

EDIT: @lojack5 tried to PM you but your box is full

[Utumno]

review e6b2e0e and links there

306:

• Move utilities and rec groups out of patcher module
  • bash/patcher/utilities.py -> bash/parsers.py (not patcher specific)
  • bash/patcher/record_groups.py -> bash/record_groups.py (not patcher specific)
• Move PatchFile, CBash_PatchFile outta bosh - hard - they use many bosh classes and globals (modInfos) - more importantly bosh is using them - this would lead to cyclic imports

307:

• move patcher game-specific variables from game/<game>/__init__.py to game/<game>/patcher/__init__.py game/<game>/constants.py and rewrite the game.template
• Edit the wiki https://github.com/wrye-bash/wrye-bash/wiki/%5Bdev%5D-Deciphering-Source-Filenames

@Sharlikran: can I proceed with those ? Can you help me with rewriting the game.template?