Skip to content

Commit

Permalink
Revert few simplifications
Browse files Browse the repository at this point in the history 
This should fix very rare but possible silent crashes.
  • Loading branch information
@JNechaevsky
JNechaevsky committed Oct 15, 2023
1 parent ae15a35 commit ae638e6
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 4 deletions.
11 changes: 8 additions & 3 deletions src/doom/r_plane.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ R_MapPlane
// angle_t angle;
fixed_t distance;
// fixed_t length;
// unsigned index;
unsigned index;
int dx, dy;

#ifdef RANGECHECK
Expand Down Expand Up @@ -177,8 +177,13 @@ R_MapPlane
ds_colormap[0] = ds_colormap[1] = fixedcolormap;
else
{
ds_colormap[0] = planezlight[BETWEEN(0, distance >> LIGHTZSHIFT, MAXLIGHTZ - 1)];
ds_colormap[1] = colormaps;
index = distance >> LIGHTZSHIFT;

if (index >= MAXLIGHTZ )
index = MAXLIGHTZ-1;

ds_colormap[0] = planezlight[index];
ds_colormap[1] = colormaps;
}

ds_y = y;
Expand Down
6 changes: 5 additions & 1 deletion src/doom/r_segs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ R_RenderMaskedSegRange
int x1,
int x2 )
{
unsigned index;
column_t* col;
int lightnum;
int texnum;
Expand Down Expand Up @@ -255,7 +256,10 @@ R_RenderMaskedSegRange
{
if (!fixedcolormap)
{
const int index = MIN(spryscale >> (LIGHTSCALESHIFT + vid_hires), MAXLIGHTSCALE - 1);
index = spryscale>>(LIGHTSCALESHIFT + vid_hires);

if (index >= MAXLIGHTSCALE )
index = MAXLIGHTSCALE-1;

// [crispy] brightmaps for mid-textures
dc_brightmap = texturebrightmap[texnum];
Expand Down

9 comments on commit ae638e6

@fabiangreffrath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, excuse my ignorance, but isn't this just a different way of writing the exact same code again?

@JNechaevsky
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really no idea what's going on. I was able to record demo with crash scene, and after this fallback to original code crash is no longer happening. Another fun thing is - getting crash on "debug" build was barely possible, though in "release" it was happing normally. Some kind of devilry. 👿

@fabiangreffrath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a red herring. Sometimes debug builds seem to allocate a bit more memory for (global? stack?) arrays. For example, I was unable to reproduce the crash in the demo linked here with a debug build, though it's clearly an array bounds violation:

fabiangreffrath/woof#1221

@JNechaevsky
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good to know, thanks! Plus OS dependency, Medusa bug in Chocolate Doom's Windows vs Linux comes to mind.

I deffinitly need to play around with this revert more, because I swear - it was able to fix recorded silent crash! I wasn't even drunk, since quit drinking enterely almost for year now. I've also go vegan, but oops, that another story. 🤭

@JNechaevsky
Copy link
Owner Author

@JNechaevsky JNechaevsky commented on ae638e6 Oct 18, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, the video record. I'm making first three demo runs with reverted, un-simplified approach, and no problem. Then I change code to simplified approach and make another three runs, and all of them causing a silent crash (program quits before demo actually ends): https://youtu.be/CaFYNh1yUYg

Not sure, but could it be related to missing if (index >= MAXLIGHTZ ) condition.

Edit: testing demo - crash.zip (requires Alien Vendetta WAD).

@fabiangreffrath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible you have messed up the u and x parts in the BETWEEN(l,u,x) macro?

international-doom/src/m_fixed.h

Line 46 in c94fe76

#define BETWEEN(l,u,x) (((l)>(x))?(l):((x)>(u))?(u):(x))

@JNechaevsky
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really no idea, it's "technically" working with both placements. But what is really crazy about this one - I can't reproduce it on my day job machine with Windows 10, no single crash in about 100 demo runs on same config. Must be a real case, where "if unsure, revert back to original code" fits just perfect, which is working fine.

@fabiangreffrath
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question was rhetoric. 😉

I think you had

ds_colormap[0] = planezlight[BETWEEN(0, distance >> LIGHTZSHIFT, MAXLIGHTZ - 1)];

where it was meant to be

ds_colormap[0] = planezlight[BETWEEN(0, MAXLIGHTZ - 1, distance >> LIGHTZSHIFT)];

@JNechaevsky
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for explanation! Still, better I'll leave original code, otherwise I will need some heart pills soon. ☠️

Please sign in to comment.