Several improvements related to Blood: What Lies Beneath

[DanielGibson]

That mod (I'll call it WLB from here on) ships a modified version of NBlood.
The author released the source of that as a .7z archive on Google Drive: https://drive.google.com/drive/folders/1q8qsPCYSRnZGCoPC602jRzjLPQO7a7Wb

According to the WLB Readme.txt, the most important change was increasing the allowed number of maps from 16 to 45 (because that mod has a lot of maps!), but there are several other changes as well.

In the current form, this PR does not make NBlood compatible with WLB, but it has several fixes and improvements I originally made when playing (and debugging) WLB.

Further down there are comments about other changes NBlood could do to better support mods like WLB without the mod having to fork NBlood.

[DanielGibson]

One thing that's a bit ugly for non-WLB is that commit that bumps BYTEVERSION - bumping should've been done with the commit breaking savegame-compat (92eebf5).
The code I added for loading older savegames works with savegames created with BYTEVERSION 105 before commit 92eebf5 (like the nblood executables shipped with WLB), but this won't work with savegames created after commit 92eebf5 (and before the bump).

[tmyqlfpir]

I don't think NBlood should be supporting one-off mod changes, unless they're part of NOONE_EXTENSIONS - ideally this would be better as a downstream fork instead.

Edit: Blood only ever supported a max of 16 levels per episode, while this hardcodes to 45. This will break vanilla INI behavior.

[DanielGibson]

ideally this would be better as a downstream fork instead

Well, according to 92eebf5#commitcomment-150376501 you don't want mod-specific forks to exist :-p

Why does it break when more than 16 levels are supported? Shouldn't less than kMaxLevels also work?

[tmyqlfpir]

ideally this would be better as a downstream fork instead

Well, according to 92eebf5#commitcomment-150376501 you don't want mod-specific forks to exist :-p

I speak for myself here, not as one of the EDuke32/NBlood devs.

Also if this was to be included, then building support for WLB should also enable NOONE_EXTENSIONS as it uses some of those sub-systems. Additionally, I would remove the hacky 105 BYTEVERSION support and just bump up the version, otherwise this would cause issues trying to support two additional versions next BYTEVERSION bump.

Why does it break when more than 16 levels are supported? Shouldn't less than kMaxLevels also work?

I would ifdef the max limit change so users who do not compile with NOONE_EXTENSIONS will only support a maximum of 16 levels. This would be important for people who are porting for memory constrained devices.

[DanielGibson]

Thanks for the suggestions!

To be clear, if the consent ends up being "we don't want mod-specific code" that's fine with me as well, I could remove the WLB-specific changes and try just to get the generic stuff (hopefully including raising the map limit) upstream - in the end, the fewer additional changes are needed to make the mod work the better it is.

But I'd like to get more feedback before I start removing anything ;)

Regarding the BYTEVERSION 105 hack, I could reduce that to just bumping the BYTEVERSION and keep the compat-code in a separate branch. I mostly implemented the hack because I want to continue using the savegames I made when I started playing WLB while I test my changes based on the latest code.
(I personally hate it when savegames stop working after an update, so in projects I'm more involved with like dhewm3 or Yamagi Quake II I try hard to avoid that. But if keeping savegames stable/backwards compatible is no goal for NBlood I'll respect that)

Hiding the increased level limit behind NOONE_EXTENSIONS sounds reasonable, I'll do that.

[tmyqlfpir]

Full disclosure - I was not aware of that discussion previously, although as a maintainer of a downstream fork of NBlood I am biased to retaining non-vanilla tweaks to their own forks.

That said, the only issues I have are the following:

1. SaveGame shouldn't be creating temps file and then renaming - it's a hacky workaround and if there is a consistent spot in the mod that crashes, it should be easy to debug and find the issue
2. Supporting older BYTEVERSION saves should not be prioritized, as future NOONE_EXTENTIONS updates will make this efforts moot and difficult to maintain
3. kMaxLevels should be bumped to 64 as to be more CPU cache aligned friendly instead of the odd number of 45, and only if NOONE_EXTENSIONS are on
4. The seq.cpp changes are strange, is this required if previous save versions support is dropped?

Thanks for taking on the effort to support this, instead of leaving the mod to be forced to use an ancient NBlood version that'll eventually become unusable.

[DanielGibson]

SaveGame shouldn't be creating temps file and then renaming - it's a hacky workaround and if there is a consistent spot in the mod that crashes, it should be easy to debug and find the issue

Sorry, this makes no sense.
When saving the game destroys my savegame because the game crashes while saving, it is harder to debug that issue, because you first need to get back to the place where it happens and hope you can trigger it again. If you still have the previous quicksave it usually is much quicker to get back to the place/state that triggers the bug so you can debug it.

kMaxLevels should be bumped to 64 as to be more CPU cache aligned friendly

I don't think the CPU cache alignment matters at all in this case, but I think that 64 is a better number anyway because it's less arbitrary and leaves some room for (even) bigger mods than WLB.

The seq.cpp changes are strange, is this required if previous save versions support is dropped?

no, that's just for the backwards-compat hack

[DanielGibson]

I rebased to latest master (removed the commit for using kMaxLevels in menu.cpp's zLevelNames because it has already been incorporated) and split the BYTEVERSION commit into two: One that bumps it to 106, the second with my backwards-compat hacks.

I also increased kMaxLevels to 64 (but only #ifdef NOONE_EXTENSIONS, otherwise it's 16 again) and enforced setting NOONE_EXTENSIONS if BLOOD_WLB is set in the Makefile in earlier commits.

[tmyqlfpir]

SaveGame shouldn't be creating temps file and then renaming - it's a hacky workaround and if there is a consistent spot in the mod that crashes, it should be easy to debug and find the issue

Sorry, this makes no sense. When saving the game destroys my savegame because the game crashes while saving, it is harder to debug that issue, because you first need to get back to the place where it happens and hope you can trigger it again. If you still have the previous quicksave it usually is much quicker to get back to the place/state that triggers the bug so you can debug it.

When NBlood saves it reinitialized AI, and this can cause crashes in itself. I've experienced multiple times when a save game does crash, it's caused by the AI being forcefully reinitialized - such as situations when an enemy was attacking and, only for the xsprite target to be cleared to -1 and crash. Raze rewrote the entire system to avoid this problem, and properly save/restore AI state.

The only case where this would help is if the crash occurred in the same tick, which is likely an error in the initialization code caused by saving game (which Blood rarely bothered to sanitize against). A number of these kind of crashes have been fixed (00107fe, 067c08e, b7b5648, 92eebf5).

This is only my opinion, and I cannot comment on NoOne's extensions given that I haven't used them myself. Maybe there is actually a useful reason to save same tick as a crash for maps that use xmap enhancements, in which case everything I've said above is moot and can be rightfully ignored.

[DanielGibson]

The crash I saw happened during saving, the savegame written was incomplete, only 3MB instead of 5MB.
I fixed it with c988664
Of course it could be that this is the only issue of that kind and all other saving-related crashes will happen in the next tick after the savegame has been written, who knows.
But I wouldn't be surprised if there are other similar cases in the code where modern mods like WLB that use the Build Engine in ways that no one thought possible in 1997 and exceed assumed limits, leading to out-of-bounds writes and thus memory corruption and crashes.

I also don't see the big deal with this, it's a pretty small change and contained to a single function, so even if it turns out that it's never needed again, it doesn't hurt anyone.

And if on the other hand similar crashes do happen in the future, this feature will spare players a lot of frustration (if I lose hours of progress because my savegame gets corrupted I'm fucking pissed and will probably stop playing the game. In the case above I was lucky to have saved to a different slot in the previous level so I didn't lose too much progress) - and will make reproducing the bug easier, as explained before.

[tmyqlfpir]

Agree to disagree then, other than that minor bike shed nitpick the changes look good. Thanks for the effort.

[DanielGibson]

Here's a nblood.exe built from this branch with BLOOD_WLB enabled: nblood-wlb.zip
It requires an installation of Blood: What Lies Beneath v1.1, just replace the nblood.exe that comes with that mod with this one.

(no idea why it's 21MB despite being a Release build, the one that ships with WLB is 6.7MB)

[DanielGibson]

@Hendricks266 @NoOneBlood I corresponded with Bloody Tom and have some more insights on the code changes that WLB does, and why, and a vague idea how to support that in a more generic way, so mods would have fewer reasons to fork your code.

(some context for others: I already wrote a bit about this at 92eebf5#commitcomment-150377613 but at that point I didn't have as much context as I do now)

Let's get the easy ones out of the way first:

He had to bump the level limit because WLB has > 40 levels, I think this can easily be done by NBlood as well, as I have in this PR. (Related to this, he also commented out an assertion; 214fcc7 fixes the underlying problem and my commits also re-enable the assertion)

The other easy change is one that indeed is specific to WLB: He replaced the Pitchfork with some kind of Spear, so there's only one tip but it should do the same damage as the four tips of the pitchfork, so in FirePitchfork() he still calls actFireVector() four times, but with identical arguments, so it's always the same impact point (and due to still being called four times it does the same damage as before). I guess allowing modders to do this in a more generic way would require using some scripting language to define weapon behavior, which probably wouldn't be exactly straightforward to implement. But maybe there is an easy way to allow this, I don't know, I'm not familiar with Build Engine modding ;)

All the other changes were done for custom items. As you can see in the changes in db.cpp, several Blood items were replaced with custom items.
Some of the replaced items were powerups, including shrink- and grow-shrooms, so most of the remaining changes are just to ensure that those items don't shrink or grow you. Even the changes in dude.cpp are just for this: He copies the values of "normal human" to "normal beast", "shrink human" and "grown human" to make sure they all behave the same (I'm not sure if all those changes would've been necessary or if disabling those items in player.cpp would've been sufficient).
In view.cpp he disables all powerups he doesn't use (and some of them correspond to items he replaced). If I understood him correctly, this is done so the unused powerups aren't shown in the HUD.

The new items that BloodyTom added for WLB don't do much. They're not powerups or similar, they're basically keys: You collect them, and if you have them in your inventory you can use some thing that you couldn't use before, and that thing might not be in the same level as the one you found the key in. Most of them are literally keys (if you use a lock while having that key, it appears in the lock and the door is unlocked, I guess that's mostly scripted in the levels?), then there's a "R'lyeh Idol" which you also collect and later put somewhere by using a pedestal (on which it then stands). Then there's a "Ghost Sphere" - not sure what that is or does - and a "Film Reel" which allows you to use a projector which then shows a short animation on a screen.
Finally there is the "Diary" which I assume are special books that stand in some places in the levels. Unlike the other items they can't be picked up, but can be used and then show a book in the players hands where a page can be read. I think BloodyTom mentioned somewhere that it's implemented like some kind of fake weapon?
Update: I just remembered, the diary probably isn't those books you can read, but another simple item that allows you to do something in a level (if you have found the diary, you can use a scroll of text in another level and Caleb is able to read it - without the diary he just says he doesn't know what it means - which then allows you to use some secret door where Caleb recites what he read on the scroll which opens that door).

Anyway, none of the new items are handled in C++ code.
So I think, apart from "not showing unused powerups in the HUD" (which I guess isn't that important) and "turn pitchfork into a spear", almost everything that WLB needs could be provided by NBlood, by providing a bunch of "custom items" that don't really do anything except from existing as an item that can be picked up and letting level scripts check if you have them in your inventory (or whatever mechanism he used exactly, I don't really know how all this works).

I imagine you could add something like kItemCustom1 to kItemCustom16 (or maybe kItemCustom32, whatever, WLB needs at least 13), maybe with values >= 151 (increase kItemMax accordingly, as far as I can tell there's lots of space until the dudes start with kDudeBase = 200,), and let modders specify the display names of those custom items in a .txt file.
All that disabling of shrinking/growing code wouldn't be necessary anymore, because all existing items could still be handled in the code (the modder just doesn't put them into the levels then).

So this is my "vague idea". It sounds relatively simple to me, but of course it's possible that in reality it's hard to do, maybe increasing kItemMax breaks code or even assets all over the place, I don't know, as I wrote, I'm not very familiar with all this, so I'm curious what you all say about this idea :)

[NoOneBlood]

Yeah, i know about all the changes he made since i'm the person who helped him to compile custom Nblood.
I still have plans to do a custom weapon system similar to what i did for custom dude type.
I don't know why he reverted shrooms behavior when he can just make a fake copy of such items (for example, using obsolete ones in list).

Last time (long long time ago) when i tried to increase kItemMax, i got desyncs in multiplayer, but it's a nice idea to provide [Items] section in episode INI where author can override names or even add new items

I still thinking about this sometimes.

[Hendricks266]

@DanielGibson Thanks for your thorough description of the changes. I lean toward not being willing to merge any BLOOD_WLB changes to the port, even temporarily, because it's a bad precedent. Fortunately, it looks like they ultimately won't be necessary. I am on board with implementing ways to customize the attributes of items and player templates, as well as exploring the use of new item slots, similar to the swcustom.txt functionality VoidSW inherits from JFSW. NoOne's suggestion of expanding blood.ini is worth exploring, but I am uncertain if it will have the right syntax capabilities compared to swcustom.txt, which uses the def parser, granting it a lot of flexibility.

If the changes to playerSizeShrink, playerSizeGrow, kItemShroomShrink, and kItemShroomGrow would be made unnecessary by using new item slots, that just leaves the change from pitchfork to spear as the only one I'm not sure how to handle.

[DanielGibson]

Ok, I'll remove the WLB specific commits from this PR and rename it to something less WLB-specific, probably tomorrow or so.

[DanielGibson]

Ok, I think this is ready for merging, I removed all the WLB-specific commits.

[Hendricks266]

Thanks! Let's continue the discussion so we can take care of everything WLB needs.

[DanielGibson]

I think one interesting question is why increasing kItemMax breaks the network protocol (or at least causes "desyncs in multiplayer").
I think if a solution for that issue can be found so you could just add (at least) around 16 custom items after the existing ones, it would already help a lot.

[NoOneBlood]

Every time when i start to think for adding something serious (like custom weapons, items, projectiles etc) i know that it will break the multiplayer. This is probably the only thing that stops me (apart from lack of time).

As a temporary "hack" WLB author can create fake items on map using modern features (aka NOONE_EXTENSIONS). It's possible to send overriden INI messages on item pickup and it's possible to create pickup screen effect using player control type (kModernPlayerControl)

@Hendricks266 if you are interested, i can try to create example of [Items] section in episode ini or just ITEMS.INI and send you in near time

[DanielGibson]

I briefly looked at the multiplayer code (network.cpp), and didn't see anything that would synchronize items - in fact, the word "item" never occurs in the whole file.
How/where is the information what items a player has etc communicated over network?

[Hendricks266]

@NoOneBlood I am interested in seeing what you come up with. Please make a branch with your experimental changes so we can review it.

All: Let's continue this discussion in #879.