MrAlaux / Nugget-Doom

Nugget Doom is a fork of Woof! with additional features.
GNU General Public License v2.0
60 stars 3 forks source link

Segfault/visual glitch with statusbar in widescreen #73

Closed kitchen-ace closed 10 months ago

kitchen-ace commented 11 months ago

Hi, plums from DoomWorld here. Decided to give nugget a try, liking it so far. I've already come across a weird bug where I got a tutti-frutti effect on the sides of the statusbar, after playing using the hud a bit. Sometimes it crashes instead, such as when viewing the automap.

nugg0000

Not sure how it happened, it won't appear normally but it will if I load this attached savegame with the attached wad. If I reset my cfg file the problem goes away. Current config is also attached. I have some mod stuff loaded but using -noautoload still causes the crash.

midnight_old.zip nuggsav0.zip nugget-doom.cfg.zip

kitchen-ace commented 11 months ago

Of note, the option "Widescreen Widget Arrangement" in the HUD options is set to "ON" in-game but they're actually off, as I switched them off earlier.

MrAlaux commented 11 months ago

I checked the WAD: it doesn't seem to replace the flat used for the Status Bar background, be it by replacing the texture itself or doing DeHackEd renaming. At least in that regard, the WAD's seemingly not the issue.

Right off the bat, it could be a savegame corruption IMO. I'm not sure how the HUD could be causing this (to clarify, the Status Bar and HUD are separate things codewise, to some degree).

@ceski-1, sorry to bother you again so soon, but could you try to get an ASAN log of the crash?

ceski-1 commented 11 months ago

@ceski-1, sorry to bother you again so soon, but could you try to get an ASAN log of the crash?

Here you go: asan.txt

I think this should fix it:

diff --git a/src/r_draw.c b/src/r_draw.c
index bb32cea..b53ef8b 100644
--- a/src/r_draw.c
+++ b/src/r_draw.c
@@ -889,7 +889,7 @@ void R_DrawBorder (int x, int y, int w, int h, int s)
   for (i = 0; i < w; i += 8)
     V_DrawPatch(x + i - WIDESCREENDELTA, y - 8, s, patch);

-  patch = W_CacheLumpName("brdr_b", PU_CACHE);
+  patch = W_CacheLumpName("brdr_b", PU_STATIC);
   for (i = 0; i < w; i += 8)
     V_DrawPatch(x + i - WIDESCREENDELTA, y + h, s, patch);
kitchen-ace commented 11 months ago

That fixes it for me, thanks @ceski-1

MrAlaux commented 10 months ago

If that's the issue, wouldn't it affect Woof too?

kitchen-ace commented 10 months ago

If that's the issue, wouldn't it affect Woof too?

I've never had it affect Woof, which is the port I have used the most in the past few months.

To be sure, I did find a way to reproduce the crash, prior to ceski-1's patch.

Config options, I'm not 100% if this is all that's required: -widescreen on (my screen is 1920x1080) -use doom font on widgets: always -widescreen widget hud off -show level time and level stats -use the hud with the numbers in the same place as the statusbar (press + once) -bind a key to "next level"

Then: -(re)start nugget -start a new game -quicksave -press "next level" -check the automap and crash

I don't have a clue why this affects Nugget and not Woof, but I can't get it to crash on Woof at all.

MrAlaux commented 10 months ago

See https://github.com/MrAlaux/Nugget-Doom/issues/71.

In short: the code fixing that issue, which is what presumably introduced this issue, was committed yesterday to both Woof and Nugget. Nugget already has an official release with that code, so assuming you tested in the latest Woof release and not its latest master, that'd be why you didn't get it in Woof (assuming that this issue will affect it, of course).

If I understand it correctly, what's going on here is that there's a "border" graphic used by both a Status Bar function and a more general border drawing function. For the former, the graphic is cached (loaded) only once elsewhere, being tagged as static, which means that it won't be automatically purged when e.g. exiting a level. For the latter, however, the graphic is cached in the function itself every time it is called, and it is tagged as cache (replacing the previous static tag), which does get automatically purged.

Based on that, I believe that a general procedure to provoke the issue is to make that border drawing function be called and then start a new level. The border graphic in use by the Status Bar is now purged. It's still loaded in, but it may be overwritten at any time when anything else is cached. As it turns out, entering the Load Game menu and loading a save from there looks like something that takes the required steps to provoke this. Something else that uses the troublesome border graphic (not sure if through that border drawing function) are small view sizes -- the ones with the Status Bar and a rock border surrounding the game world.

Anyways, I need to test this. If you'd like to do so too, you can grab an autobuild from Woof's repository here.

kitchen-ace commented 10 months ago

Aha, I actually am using Woof's git version but hadn't updated it since those last commits. Woof does indeed crash now.

Better ping @fabiangreffrath -- let me know if I should open an issue on Woof's tracker.

ceski-1 commented 10 months ago

When the game is launched, bezel is set like this:

ST_Init() --> ST_loadData() --> ST_loadGraphics() --> bezel = W_CacheLumpName("BRDR_B", PU_STATIC);

Then, when loading a saved game, patch is set like this:

G_DoLoadGame() --> R_FillBackScreen() --> R_DrawBorder() --> patch = W_CacheLumpName("brdr_b", PU_CACHE);

Then, immediately after, an attempt to draw bezel is made:

ST_refreshBackground() --> V_DrawPatch(x - WIDESCREENDELTA, 0, BG, bezel);

But bezel is no longer pointing to the correct location, so the game crashes.

So yes, Woof needs this fix too. I can make a PR.

fabiangreffrath commented 10 months ago

Yes, please.

MrAlaux commented 10 months ago

@kitchen-ace, while we wait for the PR to be merged, think you could keep using that branch instead of the master (if you aren't already) so we can test it further? I consider this a critical issue, deserving of a release that addresses it soon, but it would be unfortunate to make a new release just for this issue to make itself present again.

MrAlaux commented 10 months ago

Once again, thanks everyone!

kitchen-ace commented 10 months ago

@kitchen-ace, while we wait for the PR to be merged, think you could keep using that branch instead of the master (if you aren't already) so we can test it further? I consider this a critical issue, deserving of a release that addresses it soon, but it would be unfortunate to make a new release just for this issue to make itself present again.

I played a few maps after ceski-1's patch in this thread, and then a few more with the proper fix in the commit, and also one in Woof, and haven't encountered any crashes. I think it's quite safe to call it fixed.