Skip to content

Commit

Permalink
308 Python 3 preparation [2021/03/03] [Infernio, Utumno]
Browse files Browse the repository at this point in the history
308 is a release almost entirely focused on getting us closer to running
on Python 3. Nearly all refactoring here has the end goal of making that
transition easier, even if that may not be immediately apparent.

All commits mentioned here were authored by @Infernio or @Utumno.

-----------------------------------------------------------------------

### Records (#480)

308 development began with the merge of Part '2.5', which was already
finished in 307, but had not been merged yet for lack of testing. This
was done in bfae14e.

7e67f63 introduced a significant
refactoring of subrecord handling, bringing all sorts of improvements
like usage of AOT unpacker construction via struct.Struct, centralized
strings handling (see also #460 below), removal of lots of special
cases, etc. It also acts as a basis for most other records work that
happened in 308 and will happen in 309+.

A huge improvement came in the form of
0848872, which finally got rid of the
horrible ModFile.__getattr__. This hack (let's call it what it is)
caused fun, surprising behavior when debugging and trying to extend
ModFile's APIs every time. It also stood in the way of #460, because py3
does not like getattr with a bytestring.

d7bd007 was a merge that sprang from
the goal of refactoring getNumRecords, but soon grew to encompass larger
refactorings under #312, #460 and #480.

One final key merge in 308 was af16a5d,
which focused on the setDefault API. Wrye Bash mostly copied defaults
from xEdit, but it turns out that in many cases we don't need or want to
have defaults (e.g. for performance - see also next section). Dropping
them entirely is not feasible, however, since we do need to write out
some new records (e.g. the TES4 header, GMSTs, etc.). This merge is a
step in the direction of making defaults None unless otherwise
specified.

-----------------------------------------------------------------------

### Performance (mostly #563)

One of my personal pet peeves is how slow many of Wrye Bash's operations
still are. 309 will tackle more in this direction. These are not just
nice to haves either, they can cause serious usability problems. See
issue #563 for some information on the '0.1s threshold' and how
important it is to usability of an application. Specific improvements in
308 are:
 - c0b638b improved performance when
   viewing INI tweaks and target INIs on the INI Edits tab.
 - 9946a83 cached the target INIs on
   the INI Edits tab to reduce system calls.
 - 3d09aeb was the main merge,
   significantly improving the performance of most load order
   operations. See the merge and its commit message for details on the
   improvements.
 - 8507350 was a small commit that
   fixed one of my biggest problems with 307: the incessant flicker
   every time you tried using the global menu.
 - 4a48ae9 dropped backwards
   compatibility with 306 settings (see #376), speeding up BAIN exit
   significantly.
 - bd73ea6 made CIstr construction much
   faster, which made many BAIN operations (e.g. dragging and dropping,
   startup, exit) faster.

-----------------------------------------------------------------------

### wx (#190)

Some more de-wx'ing took place in
f707b59 and
7645e40.

In 5dbda51, we upgraded to wxPython
4.1, mostly for the high DPI code that was added in wxWidgets 3.1.x.

We were originally OK with keeping bash.py was a file that directly used
wxPython APIs, but high DPI (see #555 section below) forced us to find a
way to de-wx even it. This was accomplished in
e899dc8, which also laid the groundwork
for showing our app icons on boot popups (which we did in
538ff2a).

One major open problem with de-wx'ing was the wizards API. The problem
is that the wx.adv.Wizard class wants instances of wx.adv.WizardPage to
be passed along, but we want to encapsulate those instances entirely. In
addition, our hacky usage of 'dummy pages' did not at all fit with wx's
intended use case of the wizards API. All this came to a head when
wxPython 4.1 broke the Back button in our wizards, likely because of our
hacky usage with 'dummy' pages. Instead of monkeypatching and trying to
fix it, we threw out the entire wx.adv.Wizard API and wrote our own,
based on de-wx'd components, in
a3e6c4d. With this merge, the biggest
roadblock to fully de-wx'ing Wrye Bash is now gone.

-----------------------------------------------------------------------

### Patchers (#312)

The first massive and extremely impactful merge of 308 was
b25727b, known as 'tweak pooling'.
This is an entirely different implementation of tweaks in the Bashed
Patch that brings all sorts of improvements:
 - Thousands of lines of boilerplate code are gone
 - The pickle files we used to store injection FormIDs are gone
 - Tweaks are now almost entirely static classes (finished off a couple
   merges later)
 - Several tweakers have been ported to newer games
 - Significant performance improvements because much fewer records now
   have to be copied into the BP

The second merge focusing on patchers was
ac72709, in which the names of patcher
classes were decoupled from the keys used to store and load them from/to
BP config pickles. This allowed us to rename them to all fit one
consistent scheme. Another notable commit that was part of this merge is
eaeb831, which brings us much closer to
being able to absorb Import Cells into _APreserver as well as fixing
several issues with Import Cells.
8a9b581 was a small followup commit
that unified scanOrder and editOrder into a single patcher_order
variable.

f3b6578 followed up on tweak pooling to
make all tweaks entirely static. This brings with it huge benefits like
potential for deduplication of similar tweaks, making it much easier to
add new ones, etc.

9a1cff7 was a relatively small merge
under #312, #460 and #480 that focused on renames and using some of the
APIs introduced in earlier #312 and #480 merges to refactor more code.

-----------------------------------------------------------------------

### CBash (#530)

After having been deprecated in 307, CBash was removed entirely in
120234c. See that commit for reasoning
on why, but the short version is that it was unmaintained, cint was
poorly designed and the CBash patcher implementations were extremely
buggy.

-----------------------------------------------------------------------

### GPLv3 (#531)

Mostly a formality, but we used the 'or any later version' clause of our
GPLv2 or later license to upgrade to GPLv3 or later in
3721668. We were effectively already on
GPLv3 because of the dependencies we used, some of which would only work
if the combination of WB source code + dependency is licensed under the
GPLv3. This is just making it official.

-----------------------------------------------------------------------

### Morrowind (#479)

a5d40e1 was a merge that introduced
decoded record classes for Morrowind. We're still nowhere close to
actually reading and writing Morrowind plugins, but this is a step in
the right direction. The main reason of course is so that all the
records refactoring has to take into account Morrowind support when
designing APIs.

-----------------------------------------------------------------------

### 64bit (#481)

In 30dd357, we finally upgraded Wrye
Bash to 64bit after it had sat around on nightly for a long, long time.
The main reason for the hesitation was that some people reported cryptic
tracebacks on launch (python-lz4 was failing to import). It turned out
in the end that the fix for this was installing the MSVC 2010 x64
redistributable, which we now do as part of our installer.

64bit Python 2 gives us somewhere between a 10%-30% speedup for most
operations that we tested, most notably the Bashed Patch.

-----------------------------------------------------------------------

### Python 3 (#460)

As mentioned above, Python 3 preparation is the focus of this release.
The main hurdle here turned out to be the amount of low-level bytestring
handling that Wrye Bash does. As a result, refactoring these parts of
the code was a key goal, which resulted in patchers (#312) and records
(#480) refactoring becoming the most important open issues. Everything
described in those sections above applies to this section too.

Furthermore, we performed many targeted changes to make a Python 3
upgrade more feasible and to catch errors early. Several commits here
were originally created by @GandaG in 307:
 - 4b4cc01 cherry-picked a bunch of
   backwards-compatible changes made by 2to3.
 - 8e0fa26 dropped Path usages in
   bash.ini settings handling (see also the section on #543 below).
 - 567f153 and
   09e1633 replaced several usages of
   keys() and iterkeys() with equivalent expressions that work
   identically in py3 (e.g. next(iter(a)) intead of a.keys()[0]).
 - 83162d7 was a minor merge focusing
   mostly on getting rid of __getattr__ and __setattr__ and its cousins.
   They are slower on Python 3 than simple getattr/setattr.
 - b337e98 was a major merge focusing
   on cooperating with 2to3 to help it do better at its job. We also
   created a fork of 2to3 to push this concept even further with
   specialized fixers.
 - a75dbc6 was originally a large
   commit by @GandaG that shrank over the course of 308's development.
   It focused on dropping map/filter/range usages in favor of list
   comprehensions or xrange.

Prefixing of strings was also an ongoing task, worked on in many
commits:
 - cb6b9ce
 - 2486397
 - 83162d7
 - 1a29a01
 - 20432e6
 - 42e5795
 - 52ba47b

-----------------------------------------------------------------------

### DataStore keys (#543)

With Python 3 on the way and after a bunch of performance profiling (see
the performance section above), it became clear that bolt.Path stood in
the way of both. Thus, #543 was born - a project to make DataStore (our
class for handling file collections like mods, BSAs, INI tweaks, etc.)
use strings instead of Paths as keys. This turned out to be a gigantic
rabbit hole, leading to a ton of refactoring:
 - 74e0524 was a merge containing a
   bunch of housekeeping, but the most important change was a reduction
   in the usage of the '.s' property on Paths.
 - 129e098 renamed tons of 'path'
   occurences to make tracking down Path instances easier.
 - 5d81acd was a big merge focusing on
   following the DataStore key rabbit hole down to its event horizon.
   The resulting refactoring improved the names of many variables,
   locals, etc. and brought us much closer to absorbing BAIN structures,
   Paths instances, etc.

-----------------------------------------------------------------------

### BSA load order (#534, #546)

We had several scattered attempts to discern the order in which BSAs
load all over the codebase. 6aec0ce
centralized this handling and gave us several key improvements in the
process, including the ability for BAIN to show conflicts with BSAs
loaded via INIs, e.g. the vanilla BSAs (#546) and correct handling of
the special sVrResourceArchiveList INI setting in Skyrim VR (#534).

-----------------------------------------------------------------------

### The People tab (#548)

Was removed in 308. We used to keep it around for the purpose of keeping
the data structures generic enough, but with the ongoing refactoring, it
turned out to be too much of a maintenance burden, so it had to go.

-----------------------------------------------------------------------

### High DPI (#555)

With the upgrade to wxPython 4.1, nothing was stopping us from finally
adding high DPI support to Wrye Bash, which we did in
e899dc8. This is still not perfect, the
most notable issue is that we do not have high resolution versions of
our icons, but it's better than the blurry mess Windows displays us as
otherwise.

12571a6 was a small followup commit
that marked our installer as high DPI-aware too.

-----------------------------------------------------------------------

### env (#258)

Running Wrye Bash on Linux is a long-term goal that's very important to
us - not just for personal usage, but also for code quality. One
important part of this is abstracting over OS differences, for which we
have our env module. Unfortunately, env in 307 was in effect a
completely Windows-specific module that could only run on Linux if you
commented out large chunks of it.

All that was fixed in 19461b3, a merge
that split env.py and windows.py into an env package. Wrye Bash can now
launch of Linux and even build a BP - though it is still far from
usable, especially BAIN.

-----------------------------------------------------------------------

### Renaming (#580)

Renaming is a highly complex thing. Near the end of 308's development,
one big branch focusing on this area was merged in
5a985d8. It fixed issues with renaming
screenshots and installers, significantly improved our rename APIs and
added the ability to rename .bak files on the Saves tab. We're not
quite done with refactoring here, which is why #580 is not yet closed,
but this will hopefully be the last time we have to deal with renaming
issues (plus renaming is not really a good idea in many cases - e.g.
renaming a mod will cause serious problems if you're using it on an
existing save and power users can do it anyways by just opening the Data
folder and editing the mod in there).

-----------------------------------------------------------------------

Massive thanks to everyone who contributed to this release, including:

@Infernio, @Utumno, @GandaG, @lojack5, @Gavvers, @LordNyriox,
@Sharlikran, @Arthmoor and many more that GitHub's contribution tracker
doesn't list.
  • Loading branch information
Infernio committed Mar 3, 2021
2 parents 8e19291 + d413ea6 commit f9b5ad2
Show file tree
Hide file tree
Showing 244 changed files with 28,751 additions and 168,856 deletions.
8 changes: 8 additions & 0 deletions .github/ISSUE_TEMPLATE/config.yml
@@ -0,0 +1,8 @@
blank_issues_enabled: true
contact_links:
- name: Discord Server
url: https://discord.gg/NwWvAFR
about: Wrye Bash discord - report issues here or on AFKMods first
- name: AFKMods Thread
url: https://www.afkmods.com/index.php?/topic/4966-wrye-bash-all-games/
about: Wrye Bash forum thread - report issues here or on Discord first
54 changes: 54 additions & 0 deletions .github/issue_template.md
@@ -0,0 +1,54 @@
<!-- Please see https://github.com/wrye-bash/wrye-bash/wiki/[github]-Reporting-a-bug
for guidelines on reporting issues. In particular, you must provide the
info/steps below. Check the checkboxes by replacing [ ] with [x] and fill out
the text boxes as you follow the instructions. -->

### Latest Nightly

<!-- Download the nightly/WIP build from the second post in the AFKMods thread
or from the #wip-builds channel on Discord. If the issue no longer occurs
with this build, *do not report it!*. That means we have are aware of it and
have already fixed it. The next stable release will have the fix. -->

- [ ] I have tried the latest nightly build and the issue still occurs.

### Asking on Discord/AFKMods

<!-- IMPORTANT: Before posting an issue, be sure to ask at the official thread
on AFK Mods (https://afkmods.com/index.php?/topic/4966-wrye-bash-all-games/&do=getNewComment)
or on Discord (https://discord.gg/NwWvAFR).
Post an issue on GitHub only *after* doing so! This way we avoid tons of
duplicate issues for problems that we are already aware of/working on
fixing. -->

- [ ] I have asked on Discord/AFKMods and gotten the go-ahead to post this issue.

### BashBugDump

<!-- You must produce a bashbugdump.log and include its contents in the text
box below. See https://github.com/wrye-bash/wrye-bash/wiki/[github]-Reporting-a-bug#the-bashbugdumplog
for instructions on how to generate this file. -->

```
REPLACE THIS TEXT WITH THE CONTENTS OF THE BashBugDump.log
```

### Details of the Issue

#### Description
Replace this section with a description of the issue. Describe what you wanted
the program to do and what it did instead.

#### Step-by-step reproduction
1. Include a step-by-step process to reproduce the issue here
1. Use lines starting with '1.', GitHub will automatically number them
correctly when you submit the issue
1. If the steps require downloading mods, include links to them and the exact
versions we have to download here
1. If the steps require activating certain Bashed Patch options, include
information on your BP settings and mod tags. You can do this on the Mods
tab as follows:
* Your Load Order: 'View' > 'List Mods...'
* Your Bashed Tags: 'View' > 'List Bash Tags...'
* Your Bashed Patch config: Right click on the Bashed Patch > 'List Patch Config...'
3 changes: 3 additions & 0 deletions .github/pull_request_template.md
@@ -0,0 +1,3 @@
## DO NOT OPEN PULL REQUESTS HERE!
We do not use pull requests for Wrye Bash. Please read our [Contributing.md](https://github.com/wrye-bash/wrye-bash/blob/dev/Contributing.md)
file for instructions on how to contribute code to Wrye Bash.
15 changes: 10 additions & 5 deletions .github/workflows/pr.yaml
@@ -1,11 +1,16 @@
name: Close PR
on: pull_request
on: pull_request_target
jobs:
close:
runs-on: ubuntu-latest
steps:
- uses: roots/issue-closer@v1.1
- uses: dessant/repo-lockdown@v2
with:
repo-token: ${{secrets.GITHUB_TOKEN}}
pr-pattern: "$impossible"
pr-close-message: "@${pull_request.user.login}, pull requests are not used in this repository. See the [Contributing.md](https://github.com/wrye-bash/wrye-bash/blob/dev/Contributing.md#contributing-code) document on how to contribute code."
github-token: ${{ github.token }}
pr-comment: >
Thank you for your contribution! However, we do not use pull
requests in this repository. Please see our
[Contributing.md](https://github.com/wrye-bash/wrye-bash/blob/dev/Contributing.md#contributing-code)
document for instructions on how to contribute code.
lock-pr: false
process-only: prs
4 changes: 2 additions & 2 deletions .github/workflows/wb_ci.yaml
Expand Up @@ -14,9 +14,9 @@ jobs:
uses: actions/setup-python@v2
with:
python-version: 2.7
architecture: 'x86'
architecture: 'x64'
- name: Restore dependencies cache
uses: actions/cache@v1
uses: actions/cache@v2
with:
path: ~\AppData\Local\pip\Cache
key: ${{ runner.os }}-pip-${{ hashFiles('requirements.txt') }}
Expand Down
4 changes: 4 additions & 0 deletions .gitignore
Expand Up @@ -47,3 +47,7 @@ Mopy/bash/l10n/*NEW.txt
# pytest-cov
.coverage
.cov_html/

# Minimal LOOT masterlists - stored locally & updated manually
Mopy/taglists/
scripts/taginfo.txt

0 comments on commit f9b5ad2

Please sign in to comment.