[Discussion] Code architecture

[DavidW]

I guess the right time to talk about the architecture of the code is at the beginning. I don't want to get too picky about this, but some suggestions:

(1) Put each discrete fix or closely-associated group of fixes in its own file, with a memorable name, e.g. "some_spells_should_use_opcode_185.tpc"

(2) Put each file in a directory called 'fixes' (we can also have a 'resources' directory for game files, a 'lib' directory for function libraries, etc).

Then the tp2 component just needs to be

ACTION_BASH_FOR "%MOD_FOLDER%/fixes" ".*\.tpc" BEGIN
	WITH_SCOPE BEGIN
		INCLUDE "%BASH_FOR_FILESPEC%"
	END
END

I think that architecture is fairly easy for people to contribute to, is properly encapsulated, doesn't end up with people working on the same file unnecessarily, and is easier to test and read. Given modern WEIDU I'd strongly prefer not to just have an enormous list of patches directly in the tp2 (no criticism of fixpack intended, WEIDU has evolved out of recognition since its architecture was developed). You could separate the fixes section into BGEE, BG2EE, IWDEE, PSTEE, shared if you wanted (and indeed could pretty easily rearrange like that later in the project - that sort of change is also easier if every fix is in its own file).

(3) Use AUTO_EVAL_STRINGS. It makes for much cleaner and more readable code, but there are very rarely incompatibility issues with code not written under AUTO_EVAL_STRINGS and so it makes sense to have a definite decision to use it. (Or a definite decision not to use it, but in that case I probably won't contribute code because I'm very used to it by now.)

(4) Follow SFO's protocol of defining every spell.ids entry in the preamble as a variable, i.e. WIZARD_FIREBALL is spwi304. I think it makes code much easier to read and catches a lot of bugs. (And no-one actually has to use it!)

(5) (Possibly) use my slightly-extended versions of your *_EFFECT libraries (in, e.g., SFO) which define a bunch of bit variables and, probably more usefully, let you match on a function and patch with a function.

(6) Make the whole fixpack depend on a very large number of libraries which are all mutually dependent and which contain hundreds of idiosyncratic functions that are largely undocumented.

I'm willing to compromise on (6).

[CamDawg]

There are essentially three goals I'd like the architecture to meet, in the order of importance I place on them:

1. Ability to hand over fixes to BD for official patches
2. Maintainability
3. (3rd, but distant) Ability for players to remove/edit individual fixes from the project

In my mind, these place some restrictions on architecture, but not too many: the folder structure should match what BD uses internally for patches so that paths don't need to be altered. To my mind (apologies David and Angel) maintainability rules out SFO. AUTO_EVAL_STRINGS and the extended _EFFECT libraries advance all of these goals, so I'm on board with these. I'm ambivalent on using IDS names for spell patching: using IDS names is a slight detriment for passing fixes to BD since it forces them to include the same libraries as preamble, but it helps maintainability and player edits as long as we consistently use them. It's also mixed in terms of leveraging existing fixes: we can simply copy-and-paste fixes from IWDSpells, but not necessarily from elsewhere, so it's also a net neutral there.

Monolithic vs. compartmentalized architecture is a more delicate question. BG2FP's monolithic architecture is by design to allow players to go in and disable fixes they don't like. To that end, being able to search for, say, anything that changes Chromatic Orb by simply opening one file in an editor and searching for spwi118 (or WIZARD_CHROMATIC_ORB if we go the IDS route) is a huge boon. One of the projects I had considered working on was reviving the BG Fixpack, which more or less compartmentalizes the code in a manner similar to what you're suggesting. After both, let me just say: trying to track down what alters what is a massive PITA and makes documentation an absolute nightmare (and I know, right now, who is going to be doing 100% of that). If we had done that for BG2FP, for example, we'd have Anomen creature fixes split across at least six files. I'm willing to compartmentalize if others want to go that way, but it's strongly not my preference.

If we do compartmentalize, I'm not in favor of ACTION_BASH_FOR'ing the lot. I understand the allure of simply writing a library and dropping it into a directory for execution, but order will matter as some fixes will necessarily build upon others. I really don't want to have to finagle the alphabetical order to accomplish this.

Clustering by fix instead of by file type: cluster. BG2FP and IWDFP basically tackle fixes by file types with some multi-file patches at the end (effing effs, immunity batches), with anything involving multiple files being scrupulously commented to make up for the scattering. Internally at BD, we clustered fixes by the issue so that alone makes the case for organizing by fix, though it does mean that fixes that are order-dependent need to be clearly marked as such in the code. Anything that is copied or compiled has to go first so that patches won't get overwritten by late COPYs.

Which leads to another point: Fixpacks are designed to go first in the mod stack so take advantage of it. We don't have to REGEXP copy with 17 PATCH_IFs or dynamically build arrays: the game assets should more or less be a known quantity, so you can (generally) use fixed lists for C_Es or arrays (for BG2FP I wrote a ton of generalized C_E_Rs to find issues, then pared it down to C_E lists for the actual fixes). We can also just use straight COPYs, if it makes more sense from a maintenance standpoint, e.g. just COPY a fixed clabrn02.2da table instead of a complicated repair function to fix its rows and columns. If we have a script that we essentially need to re-write from scratch, don't try an 87-line R_T, COMPILE a new baf and overwrite the old one.

[DavidW]

OK, some persuasive points here. Thoughts:

- Agreed that matching BD setup is a priority.

- presumably, insofar as using IDS names for spell patching is concerned, if that's a problem for BD then using the extended EFFECT library is also a problem? (But if it's tolerable in both cases, great.)

- That's clearly right about ACTION_BASH_FOR; I stand corrected. We clearly need a standardized order, in which case we may as well make it explicit.

- Perhaps this is a different conversation, but I am much more skeptical than you about supporting people making their own changes, for several reasons: (i) it encourages people to think they can edit FP and then still install mods that require FP without problems, which is not at all true. (You might recall this was the same reason I didn't want IWDification to make it easy to pick and choose which spells to include.) (ii) Precisely because fixes can rely on other fixes, it can easily have unintended consequences even internal to FP. My feeling is that the gap between being competent enough to comment out bits of WEIDU code to get the right consequence, and being competent enough to write your own code (or use NI) to get the same effect, is not very large.

- I'm not sure I'm persuaded by search. (i) It's not as if it's difficult to use a tool that searches a whole mod at once. (I use Windows Grep.) (ii) Searching by file name is anyway not a reliable way to find a fix given that we might be doing a REGEXP (unless you actively want to have a strictly-no-REGEXPs policy to facilitate this, which I think is more drastic than your last paragraph implies. (And of course even FP doesn't rigidly keep everything in one file, as I found last night when I missed the 109/175 libraries.)

- That said, I don't want to be a pain about modular architecture, especially since realistically you'll be doing the bulk of coding and these are partly aesthetic calls. If you think it is better to put everything in a single megafile, that's fine. (I'd still prefer that megafile to be its own included file rather than put it directly into the tp2, but that too is partly aesthetic and not a big deal.) 

- I mostly agree with your last paragraph; having said that, there are sometimes advantages for readability, debugging, and future-patch-proofing to have the code be a little more explicit in what it's supposed to be doing. 'Patch everything that contains the string 'CDHELD' is easier to read and to check than 'patch the following 229 files [which the author of this code knows, but I don't, to be in fact the 229 files that contain the string 'CDHELD]'. (I mean: the extreme version of this would let us blindly patch entries in an extended header by raw hex address, since we know the header; I take it you wouldn't want to go that far!) I have draft code that (mostly, not quite done) implements the shifts of the immunity system we've been discussing (just to see what it looks like) and that's an example of something I think would be intolerably and unreadably complicated if done without regexp.

- Yes, obviously we shouldn't use SFO. (I don't even use it in Ascension). 

 

[Luke]

9 hours ago, DavidW said:

Then the tp2 component just needs to be

ACTION_BASH_FOR "%MOD_FOLDER%/fixes" ".*\.tpc" BEGIN
	WITH_SCOPE BEGIN
		INCLUDE "%BASH_FOR_FILESPEC%"
	END
END

To be precise, this should be slightly better

WITH_SCOPE BEGIN
	ACTION_BASH_FOR "%MOD_FOLDER%/fixes" ".*\.tpc" BEGIN
		WITH_SCOPE BEGIN
			INCLUDE "%BASH_FOR_FILESPEC%"
		END
	END
END

In so doing, you'll forget about BASH_FOR_DIRECTORY, BASH_FOR_FILESPEC, BASH_FOR_FILE, BASH_FOR_RES, BASH_FOR_EXT, BASH_FOR_SIZE when exiting from ACTION_BASH_FOR (I know you would not probably use them outside of an ACTION_BASH_FOR instruction, but just in case...)

Also, to speed up inclusions, I'd use tph as file extension (not sure if that "tpc" is a typo or not...)

9 hours ago, DavidW said:

(3) Use AUTO_EVAL_STRINGS. It makes for much cleaner and more readable code

I agree.

9 hours ago, DavidW said:

(4) Follow SFO's protocol of defining every spell.ids entry in the preamble as a variable, i.e. WIZARD_FIREBALL is spwi304. I think it makes code much easier to read and catches a lot of bugs. (And no-one actually has to use it!)

I agree (ditto for all the other IDS files).

Alternatively, if you decide not to follow SFO's protocol, I strongly suggest to use LOOKUP_IDS_SYMBOL_OF_INT, IDS_OF_SYMBOL, etc...

So for instance

WRITE_BYTE 0x272 2 // bad
WRITE_BYTE 0x272 (IDS_OF_SYMBOL ("RACE" "ELF")) // good

 

Edited March 20, 2022 by Luke

[DavidW]

47 minutes ago, Luke said:

Also, to speed up inclusions, I'd use tph as file extension (not sure if that "tpc" is a typo or not...)

It's not a typo: I use .tpc for live code, i.e. any inclusion that's not a function library. I don't think it makes a material difference to speed.

[DavidW]

47 minutes ago, Luke said:

In so doing, you'll forget about BASH_FOR_DIRECTORY, BASH_FOR_FILESPEC, BASH_FOR_FILE, BASH_FOR_RES, BASH_FOR_EXT, BASH_FOR_SIZE when exiting from ACTION_BASH_FOR (I know you would not probably use them outside of an ACTION_BASH_FOR instruction, but just in case...)

And I thought I was paranoid about encapsulation...

[CamDawg]

In no logical order at all:

4 hours ago, Luke said:

So for instance

WRITE_BYTE 0x272 2 // bad
WRITE_BYTE 0x272 (IDS_OF_SYMBOL ("RACE" "ELF")) // good

If we're at the point where race.ids has been so radically altered that I can't rely on basic race values and have to look them up every time I need to invoke them, I'm handing the Fixpack to someone else and leaving to write a Melicamp romance¹.

8 hours ago, DavidW said:

- Perhaps this is a different conversation, but I am much more skeptical than you about supporting people making their own changes, for several reasons: (i) it encourages people to think they can edit FP and then still install mods that require FP without problems, which is not at all true. (You might recall this was the same reason I didn't want IWDification to make it easy to pick and choose which spells to include.) (ii) Precisely because fixes can rely on other fixes, it can easily have unintended consequences even internal to FP. My feeling is that the gap between being competent enough to comment out bits of WEIDU code to get the right consequence, and being competent enough to write your own code (or use NI) to get the same effect, is not very large.

Sure and, AFAICT, it's not really been exercised in BG2FP or IWDFP. I guess I should clarify it's not just the idea of players being able to customize--I agree, it won't go well unless they really know what they're doing--but also a natural extension of the overall principle of transparency: the forums are (or will be) fully open, the code is (to the degree we can make it) understandable and well-commented, and there's extensive documentation down to what fixes alter which files. I like the idea of a playing a Fixpacked game, noticing a change, and then being curious enough to wonder why these changes were made. I'd hope documentation and the forums are the primary sources for answers, but I like the idea that the code itself can also be a source as well as providing a check on the team ("why isn't this change to X in the docs?").

As for not having a mess of a tp2: yes, I had already been thinking that the tp2 would essentially be a GAME_IS check, and then farm out fixes to game-specific libraries (which, in turn, use their own INCLUDEs as needed). The good news is that I cannot imagine we're going to have anything approaching the 18k monster that is BG2FP, and certainly not its 30k+ immensity before the Great Recode.

8 hours ago, DavidW said:

- presumably, insofar as using IDS names for spell patching is concerned, if that's a problem for BD then using the extended EFFECT library is also a problem? (But if it's tolerable in both cases, great.)

True enough, it shouldn't be an issue, but it is something we'll need to convey on any fix that uses this extended functionality.

As for REGEXP COPYs: it's more that they're generally unnecessary. If it's easier to C_E_R all versions of Anomen's creature file, do it--it's the all-creature kind of patches that I'm primarily worried about. If I know which, say, 100 items need a specific patch then I shouldn't poke through the other ~2000. (If it's not clear why I've selected those 100, that's a failure in documentation and commenting.) It's also a way to facilitate transfer to BD: posting a bug with 'these 100 items need a patch because of X' is better than 'this checks every item in the game and fixes it if X is detected' as the former gives the QA team a specific set of things to test². Future-proofing isn't really a concern either: when BD releases a new patch, we're going to have to re-examine everything regardless.

¹After he's been transformed back, you sickos.
² As well as making documentation easier.

[CamDawg]

Since the repo's going public tomorrow, a couple of notes:

• AUTO_EVAL_STRINGS is in.
• The ALWAYS block sets variables (0 or 1) for game_is_bgee, game_includes_sod, game_is_bg2ee, game_is_iwdee, and game_is_pstee. This allows you to just check a variable rather than using the slower GAME_IS/GAME_INCLUDES checks.
• The 'game' variable is set to bgee, bg2ee, iwdee, or pstee. This is used to load up the appropriate files for the Fixpack, e.g. the main component just INCLUDEs ~eefixpack/files/tph/%game%.tph~ and loads ~eefixpack/languages/%LANGUAGE%/%game%.tra~.
• Within each game-specific include, you can either put your fix directly there (I've added some very basic organization) or INCLUDE the fix. The latter is preferred for fixes which apply to multiple games and/or involve large numbers of files.
• I've also added a master dialogue fix file (e.g. bgee_core_fixes.d) to be COMPILEd for each platform.
• To make string changes (either for fixes or GTU), just add tra entries to the relevant tra file, e.g. add @123 if you want to update strref #123. The tra files get parsed and turned into STRING_SETs automatically.
• spell.ids is parsed into variables on all games, so you can e.g. patch Entropy Shield with either ~%CLERIC_ENTROPY_SHIELD%.spl~ or ~sppr615.spl~.
• Pretty much everything is down one directory (files) from how you generally see it in mods, e.g. rather than a 2da file being in eefixpack/2da it's instead located in eefixpack/files/2da. This is to match how BD organizes their patch structure.

Otherwise, have at it. Personally I like to see lots of comments in the code as that'll help me with documentation later--but even so I'd rather have a working, uncommented fix than nothing at all.

[CamDawg]

Repo is public.

[Luke]

@CamDawg

Are you still against the idea of using COPY_EXISTING_REGEXP...?

If that is the case, then I need to recode my pending pull requests...

[CamDawg]

Yes, still no on C_E_Rs. We're first in stack on a known platform, the patch list is known. I generally write a REGEXP to look for issues, then pare it down to a file list for a normal C_E.

[Luke]

14 minutes ago, CamDawg said:

Yes, still no on C_E_Rs. We're first in stack on a known platform, the patch list is known. I generally write a REGEXP to look for issues, then pare it down to a file list for a normal C_E.

Fine, I'll rewrite my code then...