Dynamic wiggle/Tall Sector fix for fixed-point software renderer

[kb1]

We have completed the dynamic wiggle fix code that reduces the graphical glitches mentioned in these threads:

PRBOOM Graphical Glitches
PrBoom-Plus, ver. 2.5.1.3, pg 148

Linguica posted a great demonstration .GIF in the first thread:


The WiggleFix code does a pretty good job at reducing this glitch, by fine-tuning previously-hard-coded scale factors and clamp values in the renderer. It implements what I would call a "poor man's floating-point", within the confines of 32-bit fixed-point math.

Based on how the wiggle reduction works, it is less effective on tall walls than short walls. The old WiggleHack that I posted in 2006 did a static evaluation of the entire level at level load time, to determine the tallest sector in the level. This tallest sector represents a worst-case scenario for wiggle reduction. This maximum height value was used to adjust the renderer, as much as possible, to reduce the wiggle effect.

This worked to some degree, on many IWAD and PWAD levels. But it only took one deep pit, or tall tower, to cripple the wiggle reduction ability.

This drove Entryway and I to build this new version of the WiggleHack. It dynamically adjusts the renderer on a wall-by-wall basis, to provide the most wiggle reduction possible, in each rendered frame, in all levels.

This code also permits the rendering of maximum height sectors (up to 32,767 units) without overflow or crash. At those heights, the walls are not drawn perfectly, but they are drawn. This could be considered a "limit removal" of sorts, meaning that some maps may require WiggleHack to render properly. NOTE: Sectors can be built to be up to 65,535 units tall, but Doom cannot use them, as it considers the floor to be above the ceiling in such sectors.

When using this code, there is no humanly-detectable performance loss, even on the slowest systems - the overhead is extremely small. It does require the addition of 2 32-bit ints to the sectors[] array. These do not need to be included in savegames.

Below is a complete set of instructions on how to equip a port with the latest WiggleFix. You'll have to remove any previous implementation first.

NOTE: I have not verified this, but you may need Killough's int64 sprtopscreen overflow fix. Here it is, for reference:

        // killough 3/2/98:
        //
        // This calculation used to overflow and cause crashes in Doom:
        //
        // sprtopscreen = centeryfrac - FixedMul(dc_texturemid, spryscale);
        //
        // This code fixes it, by using double-precision intermediate
        // arithmetic and by skipping the drawing of 2s normals whose
        // mapping to screen coordinates is totally out of range:

        {
          int_64_t t = ((int_64_t) centeryfrac << FRACBITS) -
            (int_64_t) dc_texturemid * spryscale;

          if (t + (int_64_t) textureheight[texnum] * spryscale < 0 ||
              t > (int_64_t) SCREENHEIGHT << FRACBITS*2)
            continue;        // skip if the texture is out of screen's range

          sprtopscreen = (long)(t >> FRACBITS);
        }

Dynamic WiggleFix Implementation - 2014/09/25

* remove any previous WiggleHack implementation, typically found here:

 1. p_setup.c: void P_SetupWiggleFix (void) { }
 2. p_setup.c:P_SetupLevel:
 	 // [kb] adjust wiggle fix as necessary
	 P_SetupWiggleFix();

 3. r_segs.c: void R_SetWiggleHack(int max_diff) { }  

* add 2 variables to sector_s struct:

struct sector_s
{
	...
	// [kb] for R_FixWiggle()
	int	cachedheight;
	int	scaleindex;
	...
}

* add lines to P_LoadSectors function:

void P_LoadSectors (int lump)
{
	...
		// [kb] for R_FixWiggle()
		ss->cachedheight = 0;
	...
}

* move R_ScaleFromGlobalAngle function to r_segs.c, above R_StoreWallRange, and modify as follows:

fixed_t R_ScaleFromGlobalAngle (angle_t visangle)
{
	int			anglea = ANG90 + (visangle - viewangle);
	int			angleb = ANG90 + (visangle - rw_normalangle);
	int			den = FixedMul(rw_distance, finesine[anglea >> ANGLETOFINESHIFT]);
	fixed_t			num = FixedMul(projectiony, finesine[angleb >> ANGLETOFINESHIFT]);
	fixed_t 		scale;
	
	if (den > (num >> 16))
	{
		scale = FixedDiv(num, den);

		// [kb] When this evaluates True, the scale is clamped,
		//  and there will be some wiggling.
		if (scale > max_rwscale)
			scale = max_rwscale;
		else if (scale < 256)
			scale = 256;
	}
	else
		scale = max_rwscale;

	return scale;
}

* Add this code block near the top of r_segs.c:

//
// R_FixWiggle()
// Dynamic wall/texture rescaler, AKA "WiggleHack II"
//  by Kurt "kb1" Baumgardner ("kb") and Andrey "Entryway" Budko ("e6y")
//
//  [kb] When the rendered view is positioned, such that the viewer is
//   looking almost parallel down a wall, the result of the scale
//   calculation in R_ScaleFromGlobalAngle becomes very large. And, the
//   taller the wall, the larger that value becomes. If these large
//   values were used as-is, subsequent calculations would overflow,
//   causing full-screen HOM, and possible program crashes.
//
//  Therefore, vanilla Doom clamps this scale calculation, preventing it
//   from becoming larger than 0x400000 (64*FRACUNIT). This number was
//   chosen carefully, to allow reasonably-tight angles, with reasonably
//   tall sectors to be rendered, within the limits of the fixed-point
//   math system being used. When the scale gets clamped, Doom cannot
//   properly render the wall, causing an undesirable wall-bending
//   effect that I call "floor wiggle". Not a crash, but still ugly.
//
//  Modern source ports offer higher video resolutions, which worsens
//   the issue. And, Doom is simply not adjusted for the taller walls
//   found in many PWADs.
//
//  This code attempts to correct these issues, by dynamically
//   adjusting the fixed-point math, and the maximum scale clamp,
//   on a wall-by-wall basis. This has 2 effects:
//
//  1. Floor wiggle is greatly reduced and/or eliminated.
//  2. Overflow is no longer possible, even in levels with maximum
//     height sectors (65535 is the theoretical height, though Doom
//     cannot handle sectors > 32767 units in height.
//
//  The code is not perfect across all situations. Some floor wiggle can
//   still be seen, and some texture strips may be slightly misaligned in
//   extreme cases. These effects cannot be corrected further, without
//   increasing the precision of various renderer variables, and, 
//   possibly, creating a noticable performance penalty.
//   

static int			max_rwscale = 64 * FRACUNIT;
static int			heightbits = 12;
static int			heightunit = (1 << 12);
static int			invhgtbits = 4;
 
static const struct
{
	int clamp;
	int heightbits;
}	
	scale_values[8] = {
		{2048 * FRACUNIT, 12}, {1024 * FRACUNIT, 12},
		{1024 * FRACUNIT, 11}, { 512 * FRACUNIT, 11},
		{ 512 * FRACUNIT, 10}, { 256 * FRACUNIT, 10},
		{ 256 * FRACUNIT,  9}, { 128 * FRACUNIT,  9}
	
};

void R_FixWiggle (sector_t *sector)
{
	static int	lastheight = 0;
	int		height = (sector->ceilingheight - sector->floorheight) >> FRACBITS;

	// disallow negative heights. using 1 forces cache initialization
	if (height < 1)
		height = 1;

	// early out?
	if (height != lastheight)
	{
		lastheight = height;

		// initialize, or handle moving sector
		if (height != sector->cachedheight)
		{
			sector->cachedheight = height;
			sector->scaleindex = 0;
			height >>= 7;

			// calculate adjustment
			while (height >>= 1)
				sector->scaleindex++;
		}

		// fine-tune renderer for this wall
		max_rwscale = scale_values[sector->scaleindex].clamp;
		heightbits = scale_values[sector->scaleindex].heightbits;
		heightunit = (1 << heightbits);
		invhgtbits = FRACBITS - heightbits;
	}
}

* add this line, in r_segs.c:R_StoreWallRange, right before calls to R_ScaleFromGlobalAngle:

	R_FixWiggle(frontsector);

* In r_segs.c:R_StoreWallRange, move the following 2 lines down below the calls to R_ScaleFromGlobalAngle:

	worldtop >>= 4;
	worldbottom >>= 4;

(the "4" will soon be changed to "invhgtbits" - see below)

* do each of the following/Search and Replace:
* Change all instances of "HEIGHTBITS" to "heightbits"
* Change all instances of "HEIGHTUNIT" to "heightunit"
* Change all instances of ">>=4" in r_segs.c to ">>= invhgtbits"
* Change all instances of ">>4" in r_segs.c to ">> invhgtbits"


That's it!

If you implement this in your port of choice, please reply with any changes you had to make to get it to compile and work. And, of course, please reply, and let us know how it works for you.
Enjoy!

[bradharding]

Great work guys! An invaluable addition to Doom Retro, I've implemented it straight away without any trouble. It appears to work fine!

[jeff-d]

This is great stuff guys!

I've copied the changes to my source port.
https://github.com/jeffdoggett/Doom/commit/f5cc94b408cd23219c99d2e745e5aa2b07ff55e9

Also the code to initialise cachedheight to zero in P_LoadSectors isn't necessary because there's a memset(x,0,s) above.

[Memfis]

Very nice work, the double yellow line in DOOMCITY.WAD looks perfect now!
(before - after)

[Quasar]

Good to at last see a mathematically rigorous solution that works when placed directly into the vanilla renderer. I am pleased, even though this doesn't benefit me directly.

[fabian]

kb1 said:

Below is a complete set of instructions on how to equip a port with the latest WiggleFix. You'll have to remove any previous implementation first.

Thank you for the very detailed instructions. This code will most probably make its way into the next Crispy Doom release.

However, I found the instructions a bit Boom-centric. Please find my remarks from a Chocolate Doom point of view below:

I have not verified this, but you may need Killough's int64 sprtopscreen overflow fix. Here it is, for reference:

It would have been nice to know that this replaces the single line reading

sprtopscreen = centeryfrac - FixedMul(dc_texturemid, spryscale);

in R_RenderMaskedSegRange in r_segs.c.

* add 2 variables to sector_s struct:

which in Vanilla/Choco is sector_t in r_defs.h.

* add lines to P_LoadSectors function:

As has already been pointed out, this is not strictly necessary, as the entire "sectors" array gets memset to 0 a few lines earlier.

fixed_t R_ScaleFromGlobalAngle (angle_t visangle)
{
	[...]
	fixed_t			num = FixedMul(projectiony, finesine[angleb >> ANGLETOFINESHIFT]);

Edit: Vanilla/Choco has "projection" instead of "projectiony".

* In r_segs.c: R_StoreWallRange, move the following 2 lines down below the calls to R_ScaleFromGlobalAngle:

In Vanilla/Choco it appears that those two lines are already below the calls to R_ScaleFromGlobalAngle.

* do each of the following/Search and Replace:
* Change all instances of "HEIGHTBITS" to "heightbits"
* Change all instances of "HEIGHTUNIT" to "heightunit"

If you are not careful, you might end up replacing the

#define HEIGHTBITS 12

and

#define HEIGHTUNIT (1<<HEIGHTBITS)

lines as well which might render the WiggleFix useless. Better remove them before blindly applying Search & Replace.

* Change all instances of ">>=4" in r_segs.c to ">>= invhgtbits"
* Change all instances of ">>4" in r_segs.c to ">> invhgtbits"

In Choco, these were ">>= 4" (with a space) and ">>4" (without a space).

Well, thanks again for posting this! However, I am a ibt disappointed that this doesn't fix the wiggling wall in the corners of nuts.wad, but I assume that the wall is simply too long and the scale had to get clamped just as before, right?

[kb1]

Thanks, guys - I am very glad to have been helpful. Just remember these 2 points:

1. Quasar's and SOM's renderer will probably always be better!
2. It won't fix everything.

@Fabien:
All very good points. Developers: Please read Fabian's points: My instructions were as good as I could do, from memory, without providing source. Your port may vary.

By the way, I added the initialization in P_LoadSectors to make it clear that those variables need to be initialized. It's not strictly necessary.

Nuts.wad: Could be "long wall error". I'll have to check it out. That's the tradeoff of using integers vs. floating point: Very small, and very large, are not represented. Anytime you see wiggling, texture alignment quivering, walls moving in and out as you move, you can bet it's due to fixed-point integer math.

[kb1]

A point I forgot to mention: The wiggle fixing code also works on 3d floors, so ports like Legacy could benefit from it as well. Basically, if your code uses HEIGHTBITS, the WiggleFix will probably work with it.

[Shadow Hog]

Actually, I'm not so sure about that. I tried putting the code into the Legacy-based SRB2 over the weekend, but it seemed to introduce more visual bugs with FOFs than were present without the wiggle fix.

First it was some sort of lighting glitchery - compare without the bug fix to with it. Notice how the Light Block is slanting upwards with it in place? Still, the fix for that was relatively simple; change some of the ">>invhgtbits" and ">>=invhgtbits" back to ">>4" and ">>=4".

The next one, though, I couldn't really find a fix for: this. Essentially, the tops of FOFs would periodically vanish in specific places. This particular case seems to be caused by the fact the FOF has a very large, thin hole in it on the far right of this screen, a hole that was definitely victim to the wiggling without the bug fix. This visual artifact wasn't only present here, either; it was showing up all over the place, although generally you'd move so fast through the offending areas that it'd only show up for a frame or two at most. You could make it linger if you figured out the exact position and angle you had to look to cause it to appear in the first place, however...

This one, I couldn't figure out the cause of; all I knew is it didn't show up when the wiggle fix wasn't in, and occasionally did when it was. So basically, I had to decide whether the wiggling visual glitch was more important than the tops of FOFs suddenly vanishing periodically, and ultimately decided introducing a rather noticeable bug to fix one that hadn't been bothering most people simply wasn't a worthwhile trade-off.

Granted, SRB2 is in far too many respects built on a foundation of band-aids atop band-aids, and I am not particularly knowledgeable about the vanilla Doom renderer code, let alone SRB2's, but still, here we are. I suppose your claim that it'd work with FOFs in Legacy itself remains to be proven, but it didn't seem to play nice with them in our Legacy variant.

For what it's worth, though, the wiggle-fixing definitely worked outside the context of FOFs, as illustrated when I briefly ported Doom 2's MAP01 to the game just to test it: without fix, with fix.

(I'd also kinda hoped this would fix the wiggling with ultra-long linedefs, but alas, it didn't. I'm guessing that's a separate bug entirely...)

[kb1]

Shadow Hog said:

Actually, I'm not so sure about that. I tried putting the code into the Legacy-based SRB2 over the weekend, but it seemed to introduce more visual bugs with FOFs than were present without the wiggle fix.

I can confirm that it works in my home port, which has my best attempt at implementation of Legacy 3d floors. What is SRB? Can I download it? With source?

Shadow Hog said:

First it was some sort of lighting glitchery - compare without the bug fix to with it. Notice how the Light Block is slanting upwards with it in place? Still, the fix for that was relatively simple; change some of the ">>invhgtbits" and ">>=invhgtbits" back to ">>4" and ">>=4".

Honestly, they look different, but I cannot see a glitch, specifically, cause I can't really tell what's going on with the shadow. That shadow effect is very cool, by the way! NOTE: If you're only putting some of the ">>invhgtbits" back to >>4, you're using the wrong scale in places. It must be all or nothing. You need to be sure that you're using >>invhgtbits, >>=invhgtbits, max_rwscale, heightunit, and heightbits everywhere. I'd like to see the source (what is SRB again?), so maybe I can spot something.

Shadow Hog said:

The next one, though, I couldn't really find a fix for: this. Essentially, the tops of FOFs would periodically vanish in specific places. This particular case seems to be caused by the fact the FOF has a very large, thin hole in it on the far right of this screen, a hole that was definitely victim to the wiggling without the bug fix. This visual artifact wasn't only present here, either; it was showing up all over the place, although generally you'd move so fast through the offending areas that it'd only show up for a frame or two at most. You could make it linger if you figured out the exact position and angle you had to look to cause it to appear in the first place, however...

This one, I couldn't figure out the cause of; all I knew is it didn't show up when the wiggle fix wasn't in, and occasionally did when it was. So basically, I had to decide whether the wiggling visual glitch was more important than the tops of FOFs suddenly vanishing periodically, and ultimately decided introducing a rather noticeable bug to fix one that hadn't been bothering most people simply wasn't a worthwhile trade-off.

Granted, SRB2 is in far too many respects built on a foundation of band-aids atop band-aids, and I am not particularly knowledgeable about the vanilla Doom renderer code, let alone SRB2's, but still, here we are. I suppose your claim that it'd work with FOFs in Legacy itself remains to be proven, but it didn't seem to play nice with them in our Legacy variant.

For what it's worth, though, the wiggle-fixing definitely worked outside the context of FOFs, as illustrated when I briefly ported Doom 2's MAP01 to the game just to test it: without fix, with fix.

(I'd also kinda hoped this would fix the wiggling with ultra-long linedefs, but alas, it didn't. I'm guessing that's a separate bug entirely...)

The Lagacy 3d floor code itself has some very nasty rendering bugs. I haven't studied it enough, but the way it determines the Z (depth from viewer) coordinate is suspect. Legacy floors are drawn like strips of sprites, not unlike the 2-s wall code. But, simple sprite sort no longer works, and, in fact, there was some code to split up sprites horizontally, for clipping, and for colormapping each horizontal chunk of sprite, like a sprite half-way out of deep translucent water.

The wiggling code rescales the renderer, and, if done globally, the effect of the rescaling is essentially "cancelled out". But, again, Legacy uses the texture scale to approximate a Z-coordinate, so it can sort sprites and 3d floor sides.

The only real proof I can provide you is that, in my home port, I use the wiggle code with 3d floors, with no more problems than I had without it, so I have to guess that a step was missed in SRB. Having said that, I did have to fix some more glaring Legacy 3d floor bugs. I am sure that some of those fixes happened before WiggleFix, and some after.

As cool as 3d floors are, implementing them in the existing renderer is a massive task, because the code needs to get injected all over the place, and it's real easy to miss something. And, even if you implement them properly, they cause all sorts of weird rendering issues, like sprites popping in front of, then behind the 3d floors, especially when a sprite is coming up through a hole in the 3d floor.

I would love to spend some real time and focus on new 3d floor code.

To summarize, yes, I still believe that the 3d floor code can work with the Wiggle code, and be enhanced by it. I strongly suspect that something was overlooked. Best of luck!

[Shadow Hog]

Well, the game's here, but the source code, the part you probably actually care about, is available here. The files I modified, with modifications in place, are in a zip file here. Technically I was working against a private SVN copy thereof, but, other than p_setup.c (which IIRC only has the P_LoadSectors() tweak), it doesn't look like any of the files in question had been modified since our last public release, so you could probably just plug them straight in.

(As for the glitch with the shadow, it's more apparent when you see it in-game than in a shrunken GIF, but every lighting change a 3D floor causes, whether it's from the shadow beneath or because the 3D floor has a Legacy-style colormap associated with it, is offset from where the 3D floor actually is, appearing to slant upwards the further away from the camera it gets. It's pretty trippy, especially in levels that make excessive use of said colormap feature. If you wanted to see it for yourself, change the "4"s back to "invhgtbit"s in R_StoreWallRange.)

[_bruce_]

Very interesting - thanks a lot!

[kb1]

Oh, Sonic Robo Blast! Looks fun! Give me a few days, and I see if I can figure out what's happening.

[wesleyjohnson]

It is hard to tell from this forum posting, but it appears that that the scale has a bit shift that changes during the rendering.
This will not work with DoomLegacy because those scales are saved for sprites, line segments, and floors, and are used later to determine draw order for 3d floors. There is an array of scale that is saved for every line segment drawn. It is saved only for the frame draw duration, so a scale factor change could defer to the next frame.

It would be better to go to a 64 bit scale or a float scale, but it would have to be changed everywhere in the render code. I think there are even some scale uses in the r_draw files.

In those GIFs, it would help to know what was generating that black bar. If it is a light only 3dfloor, then that draw is deferred to the 3d floor sorted draw with the sprites. The floor draw saves a scale increment because the scale is not constant across the draw of a 3d floor, and that looks to have the wrong bit shift scale at the time of the actual draw.

DoomLegacy 1.45.2 is in process of release. If you have older versions of 3d floors then there are some bugs that were fixed in 1.45. One fix is for draw order in stairs made with 3d floors. There is still one bug with sprite corpses and 3dfloor railings that defies fixing, because it depends on the use of BSP tree rendering order.

There is more work to be done on that, and one consideration is whether to save a true distance measure instead of the scale. Sometimes I feel that I have only wounded the bugs. From my many attempts in this area, I know that wild drawing can result from messing with the saved scales.

[Shadow Hog]

wesleyjohnson said:

In those GIFs, it would help to know what was generating that black bar. If it is a light only 3dfloor, then that draw is deferred to the 3d floor sorted draw with the sprites. The floor draw saves a scale increment because the scale is not constant across the draw of a 3d floor, and that looks to have the wrong bit shift scale at the time of the actual draw.

Just for clarification, yes, the black bar is the result of a light-only 3D floor touching various walls.

[kb1]

wesleyjohnson said:

It is hard to tell from this forum posting, but it appears that that the scale has a bit shift that changes during the rendering.
This will not work with DoomLegacy because those scales are saved for sprites, line segments, and floors, and are used later to determine draw order for 3d floors. There is an array of scale that is saved for every line segment drawn. It is saved only for the frame draw duration, so a scale factor change could defer to the next frame.

It would be better to go to a 64 bit scale or a float scale, but it would have to be changed everywhere in the render code. I think there are even some scale uses in the r_draw files.

I definitely have the wiggle fix working with my variant of Legacy 3d floors. Yes, there's scaling in the r_draw code, and it is supposed to get a WiggleFix applied to it as well. Lower in my post, I state that I believe it's the reason SRB is having problems.

wesleyjohnson said:

DoomLegacy 1.45.2 is in process of release. If you have older versions of 3d floors then there are some bugs that were fixed in 1.45.

Nice! I'll have to check that out.

wesleyjohnson said:

There is more work to be done on that, and one consideration is whether to save a true distance measure instead of the scale. Sometimes I feel that I have only wounded the bugs. From my many attempts in this area, I know that wild drawing can result from messing with the saved scales.

I know exactly what you mean, and you describe that feeling perfectly!

I have suspected that the per-slice scaling stuff is sketchy, and that it suffers from inaccuracies that cause some of the strange sort order issues. It requires more patience and effort than I've been willing to exert so far. I really won't be satisfied until 3d floor hidden surface removal works as well as the rest of Doom rendering (which is not 100% correct either, especially with sprite clipping).

SRB

As far as SRB goes, holy moly, that is one advanced source port! Wow! I will be studying this code base for a long time. You guys are doing some neat stuff there :)

Ok, I took a brief look at the "here's the modules I changed" source, and, though I did not find a direct solution for you, I would like to highlight some areas for you to check further.

1. In LoadLineDefs2, you have this repeatcnt thing:

	sides[ld->sidenum[0]].repeatcnt = (INT16)(((unsigned)sides[ld->sidenum[0]].textureoffset >> FRACBITS) >> 12);
	sides[ld->sidenum[0]].textureoffset = (((unsigned)sides[ld->sidenum[0]].textureoffset >> FRACBITS) & 2047) << FRACBITS;
	sides[ld->sidenum[1]].repeatcnt = (INT16)(((unsigned)sides[ld->sidenum[1]].textureoffset >> FRACBITS) >> 12);
	sides[ld->sidenum[1]].textureoffset = (((unsigned)sides[ld->sidenum[1]].textureoffset >> FRACBITS) & 2047) << FRACBITS;

That >> 12 looks suspiciously like it should be >> heightbits, but I could be totally wrong. What's going on here?


2. This code scares me a bit:

	//Handle over/underflows before they happen.  This fixes the textures part of the FOF rendering bug.
	//...for the most part, anyway.
	if (((signed)dc_texturemid > 0 && (spryscale>>FRACBITS > INT32_MAX / (signed)dc_texturemid))
	 || ((signed)dc_texturemid < 0 && (spryscale) && (signed)(dc_texturemid)>>FRACBITS < (INT32_MIN / spryscale)))
	{
		spryscale += rw_scalestep;
		continue;
	}

Is it necessary, and/or, is it to fix the wiggle? It's kinda slow looking, and gives me the feeling that it is doing checking that should not need to be done. That's just my feelings, for whatever they're worth :)

3. Your code seems to be missing some "light caster" stuff that's in my code, and original Legacy code, and concerns light, and shadowing, and invhgtbits:

    if (frontsector->numlights)
    {
		dc_numlights = frontsector->numlights;
		
		if (dc_numlights >= dc_maxlights)
		{
			dc_maxlights = dc_numlights;
			dc_lightlist = realloc(dc_lightlist, sizeof(r_lightlist_t) * dc_maxlights);
		}
		
		for (i = p = 0; i < dc_numlights; i++)
		{
			light = &frontsector->lightlist[i];
			rlight = &dc_lightlist[p];
			
			if (i)
			{
				if (light->height < frontsector->floorheight)
					continue;
				
				if (light->height > frontsector->ceilingheight)
				{
					if (i + 1 < dc_numlights &&
						frontsector->lightlist[i + 1].height >
						frontsector->ceilingheight)
						continue;
				}
			}

			rlight->height = (centeryfrac >> invhgtbits) -
					FixedMul((light->height - viewz) >>
						invhgtbits, rw_scale);

			rlight->heightstep = 
				-FixedMul(rw_scalestep, (light->height - viewz) >> invhgtbits);

			rlight->flags = light->flags;
			
			if (light->caster && light->caster->flags & (FF_SOLID | FF_WALKABLE))
			{
				lheight = *light->caster->bottomheight >
					frontsector->ceilingheight ?
					frontsector->ceilingheight + FRACUNIT :
					*light->caster->bottomheight;

				rlight->botheight = (centeryfrac >> invhgtbits) -
					FixedMul((*light->caster->bottomheight - viewz) >>
						invhgtbits, rw_scale);

				rlight->botheightstep = -FixedMul(rw_scalestep,
					(*light->caster->bottomheight - viewz) >> invhgtbits);
			}
			
			rlight->lightlevel = *light->lightlevel;
			rlight->extra_colormap = light->extra_colormap;
			rlight->extra_colortype = light->extra_colortype;
			p++;
		}
		
		dc_numlights = p;
    }

Are you handling this a different way, or somewhere other than r_segs.c?


4. And, the kicker. I think this might be what you're missing, but I didn't look at your r_draw.c:
r_draw.c: Legacy function R_DrawColumnShadowed(8) requires 2 sets of heightbits replacements, like this below. There's two blocks of code there, which look like the block I list below. They both require wiggle fixing. It fact, in my source I had to take "static" off of the definition of heightbits, for this very reason.

	// SoM: This runs through the lightlist from top to bottom and cuts up
	// the column accordingly.
	for (i = 0; i < dc_numlights; i++)
	{
		// If the height of the light is above the column, get the colormap
		// anyway because the lighting of the top should be affected.
		solid = dc_lightlist[i].flags & FF_CUTSOLIDS;
		
		height = dc_lightlist[i].height >> heightbits;
		
		if (solid)
			bheight = dc_lightlist[i].botheight >> heightbits;

All the rest of your wiggle implementation looks spot on to me. Your port is very interesting - you guys have done a lot of work, here! Is there a generic Doom port in the works, too? I'd love to see this in a general port. How would you describe the lineage of your port? From what I can tell, it's Boom+Legacy+ZDoom, with lots of original additions.

Good luck on the wiggle fix. From what I can tell, you're 99% there.

By the way, please let me know if this solves your issue, or if you get it solved another way.

[fabian]

kb1 said:

Nuts.wad: Could be "long wall error". I'll have to check it out.

Any ideas about that?

[kb1]

fabian said:

Any ideas about that?

Not yet. Seems very related to the wiggle fix, though. I'm not sure - the WiggleFix may induce long wall error, as a side-effect of the way it works. I can't say that I really studied the WiggleFix against a known "long wall error" wall. But, I will, especially now that I have a known "long wall error" wall to study. Unfortunately, Real Life is working me over right now, so it'll take some time, before I can devote any time.

A nice test would be to make the WiggleFix toggleable in-game, with a single keypress, walk up to the wall, toggle, and notice the effect.

[kb1]

@Shadow Hog:

Just curious if you checked your DrawShadowColumn implementation, and/or if you found what was causing your issue. Also, does your team have any plans of releasing your port as a non-SRB-specific generic Doom port? It just seems to be very feature rich, moreso than many other ports - it would be interesting.

[wesleyjohnson]

Doom Legacy 1.42.2 has been released. Source and Linux binaries are ready. Win32 binaries are in the process of being posted.

I have been using special wads and instrumented code to investigate the problem with sprite corpses on 3d floors. I have found it happens because of the order the BSP inserts the floors into the segnode lists.
When a sprite is on the border of two BSP depth branches, the render order cannot be satisfied for both BSP branches at the same time.
This often happens when a sprite corpse is on a 3d floor, but is partly under a railing that uses a 3d floor too. This particular construction defeats my 3d floor sorting solutions.
Attempts to sort the inserts cause all kinds of render order problems, usually of seeing objects through floors. The BSP insert order is providing some kind of ordering and I have not found a sort to replace it. All this code uses the scale to determine draw order.

It you are changing the scale bit shift dynamically during the render, I find it difficult to see how you fix all the scales that were already saved in arrays and in segnodes. It is expensive to go back and fix previous segnodes and seg arrays.

[kb1]

wesleyjohnson said:

If you are changing the scale bit shift dynamically during the render, I find it difficult to see how you fix all the scales that were already saved in arrays and in segnodes. It is expensive to go back and fix previous segnodes and seg arrays.

You may be right. Honestly, I did not investigate it enough to say one way or another. I looked around a 3d floor level a bit, and it looked ok and had no wiggling. The previous wiggle-fixing code did so once per level, so this was not a problem for 3d floors. Even once per frame would be ok. But, now, it's once per wall. Then again, it may still be ok, I just cannot say. A possible solution is to store an unscaled version of the "distance". That code always seemed strange to me - If I'm not mistaken, it is saving the texture stretch, not the true distance. I can remember trying to understand the sprite sorting myself, way back when, and found that distance array to be unbelievable, and unreliable when sorting sprites. I just don't understand it enough.

Back to your specific issue, maybe you need to use the sprite splitting code, and sort each fragment separately?

I do plan to investigate this issue further - 3d floors are so close to being correct, that it's worth it to get them working perfectly. If I find out anything, I'll let you know.

[wesleyjohnson]

Tests of the 3dfloors in DoomLegacy need to be of sufficiently complicated scenes that the BSP render order conflicts with the distance sort order.

For debugging I use some custom wads with three floors floating staircases and multiple objects and view positions. I tag specific floors and instrument the code to trigger on those tags.

Tests use saved positions in hth2.wad, where there are over 20 3d floors in the same room and many objects. The objects and monsters two floors below become visible through the floors with most changes to the 3d floor code.

Edit: Yes splitting the sprites vertically is one of the solutions I have been working on. Tests of the idea did not work out and it was getting complicated fast. I just ran out of time for this release. The existing sprite splitting code is a horz. split and that does not help.

[fabian]

Hey kb1,

I think I found a bug in the WiggleFix code -- or I found one in Crispy Doom and it is just triggered by the WiggleFix. It occurs if you load NOVA.WAD, warp to map30 and simply go backwards. The game will then crash with the following error message (sounds familiar?):

R_MapPlane: 146, 146 at 65514

The numbers are often different, but the third one is always close to 0xFFFF. I can cure this issue if I apply the following patch, which merely reduces the maximum clamp value to half:

--- a/src/doom/r_segs.c
+++ b/src/doom/r_segs.c
@@ -139,7 +139,7 @@ static const struct
     int clamp;
     int heightbits;
 } scale_values[8] = {
-    {2048 * FRACUNIT, 12},
+    {1024 * FRACUNIT, 12},
     {1024 * FRACUNIT, 12},
     {1024 * FRACUNIT, 11},
     { 512 * FRACUNIT, 11},

Any idea what could cause this?

Thank you already!

- Fabian

[bradharding]

fabian said:

Hey kb1,

I think I found a bug in the WiggleFix code -- or I found one in Crispy Doom and it is just triggered by the WiggleFix. It occurs if you load NOVA.WAD, warp to map30 and simply go backwards.

If it helps, I've previously implemented this fix in Doom Retro, and had to test this myself: load NOVA.WAD, idclev30, walk backwards in the small room you start with the switch ahead of you: no crash. No graphic anomalies either. So perhaps not an issue with this code?

[fabian]

bradharding said:

If it helps,

Unfortunately not, since you have already replaced integral parts of the renderer with implementations taken from Boom or elsewhere. And the "reference implementation" of the WiggleFix is also based on PrBoom+.

So, either in Vanilla the rwscale limit is really at 1024 or I have overseen something crucial. :/

[kb1]

fabian said:

Hey kb1,

I think I found a bug in the WiggleFix code -- or I found one in Crispy Doom and it is just triggered by the WiggleFix. It occurs if you load NOVA.WAD, warp to map30 and simply go backwards. The game will then crash with the following error message (sounds familiar?):

R_MapPlane: 146, 146 at 65514

The numbers are often different, but the third one is always close to 0xFFFF. I can cure this issue if I apply the following patch, which merely reduces the maximum clamp value to half:

--- a/src/doom/r_segs.c
+++ b/src/doom/r_segs.c
@@ -139,7 +139,7 @@ static const struct
     int clamp;
     int heightbits;
 } scale_values[8] = {
-    {2048 * FRACUNIT, 12},
+    {1024 * FRACUNIT, 12},
     {1024 * FRACUNIT, 12},
     {1024 * FRACUNIT, 11},
     { 512 * FRACUNIT, 11},

Any idea what could cause this?

Thank you already!

- Fabian

I can't check right now, so let me ask a few questions:

1. Is that a vanilla error message?
2. What are the variables being shown in that MapPlane error?
3. What conditional statement makes that error appear?
4. Did you implement Killough's int64 centeryfrac fix from Boom?

If not, then, yes, you may need to run with the first column divided by 2. Or, better yet, add Killough's fix :) Apparently you are experiencing an overflow. 65514 = -22 signed, which suggests int rollover. We tweaked the code to run right up against the limits, actually much more often than Vanilla. I know that, in my source and in PrBoom+, when overflow occurs, the screen is filled with flats that stretch to the horizon - instead of the MapPlane error. We may have independently added bounds checking as well, but I can't check right now. I think the int64 math is what's keeping the crash from occurring.

Please let me know. I was hoping to find some test case for needing Killough's code, and, I think you may have found it. I'll try NOVA.WAD tonight, as well.

EDIT: If your implementation was wrong, it would most likely be obvious. In other words, if the "wiggle" has stopped for you, I think you have implemented it correctly. But, you now need the above-mentioned int64 fix, replacing a 32-bit calculation with a 64-bit calc that handles the larger edge-case values.

[fabian]

kb1 said:

1. Is that a vanilla error message?

Yes, it's from P_MapPlane() when the RANGECHECK macro is #defined.

2. What are the variables being shown in that MapPlane error?
3. What conditional statement makes that error appear?

#ifdef RANGECHECK
    if (x2 < x1
     || x1 < 0
     || x2 >= viewwidth
     || y > viewheight)
    {
	I_Error ("R_MapPlane: %i, %i at %i",x1,x2,y);
    }
#endif

4. Did you implement Killough's int64 centeryfrac fix from Boom?

Yes, I already did. Right before I applied the WiggleFix patching.

If not, then, yes, you may need to run with the first column divided by 2. Or, better yet, add Killough's fix :) Apparently you are experiencing an overflow. 65514 = -22 signed, which suggests int rollover.

Yes, it's pretty sure an int overflow. And I need to find it...

EDIT: If your implementation was wrong, it would most likely be obvious. In other words, if the "wiggle" has stopped for you, I think you have implemented it correctly. But, you now need the above-mentioned int64 fix, replacing a 32-bit calculation with a 64-bit calc that handles the larger edge-case values.

No, the wiggle is gone and it works for each map I have tried so far, even critical ones like INVASION.WAD E1M3. The crash in NOVA.WAD MAP30 is the first one I experienced so far, and it's a pretty extreme level, so say the least. And so far it only occured if I walked straight backwards from the start.

[Quasar]

fabian said:

No, the wiggle is gone and it works for each map I have tried so far, even critical ones like INVASION.WAD E1M3. The crash in NOVA.WAD MAP30 is the first one I experienced so far, and it's a pretty extreme level, so say the least. And so far it only occured if I walked straight backwards from the start.

Are you near a 2S line with a greater than 2048 unit difference between floor and ceiling?

[fabian]

Hm, no, the height differences in the immediate neighborhood are all < 1024.

[kb1]

It's very strange, indeed - you should not be able to overflow with those numbers. Is y a 16-bit short in your implementation? I know I grew some of those to 32-bit, and your error suggests a 16-bit overflow. I guess I should have done a better audit against vanilla. Can you show me some renderer source code that I can use to compare against? Maybe that's exactly what's happening.

I'd be curious to remove the MapPlane check temporarily, and replace it with:

if (y < 0) y = 0;

Just to see what happens. The more I think about it, the more I believe that I did, in fact change some shorts to ints, and that that could be the cause of this.

Here's my error check:

#ifdef RANGECHECK
	if (x2 < x1	|| x1 < 0 || x2 >= viewwidth || y >= viewheight || y < 0)
		I_Error("R_MapPlane: %i, %i at %i",x1, x2, y);
#endif

In my source, MapPlane's arguments are ints. And, my check checks for y < 0, which, if you're using 16-bit ints, your check for y > viewheight would do an unsigned check for a negative y, so, I'm a bit confused.

Also, these are ints:

int centery;
// [kb] changed to int to handle dropoff overflow
int negonearray[MAXWIDTH];
int screenheightarray[MAXWIDTH];
int r_dsclipbot[MAXWIDTH];
int r_dscliptop[MAXWIDTH];
int *mfloorclip;
int *mceilingclip;

fixed_t spryscale;
fixed_t	sprtopscreen;
fixed_t	sprbotscreen;

Also, check out this portion of *R_CheckPlane:

visplane_t *R_CheckPlane(visplane_t *pl, int start, int stop)
{
	int intrl, intrh, unionl, unionh, x;
	
	if (start < pl->minx)
		intrl   = pl->minx, unionl = start;
	else
		unionl  = pl->minx,  intrl = start;
	
	if (stop  > pl->maxx)
		intrh   = pl->maxx, unionh = stop;
	else
		unionh  = pl->maxx, intrh  = stop;
	
	// dropoff overflow / make 32-bit
	for (x = intrl; x <= intrh && pl->top[x] == 0xffffffffu; x++)
		;

And:

static void R_MakeSpans(int x, unsigned int t1, unsigned int b1, unsigned int t2, unsigned int b2) //dropoff overflow
{
	for (; t1 < t2 && t1 <= b1; ++t1)
		R_MapPlane(t1, spanstart[t1], x - 1);

	for (; b1 > b2 && b1 >= t1; --b1)
		R_MapPlane(b1, spanstart[b1], x - 1);

	while (t2 < t1 && t2 <= b2)
		spanstart[t2++] = x;

	while (b2 > b1 && b2 >= t2)
		spanstart[b2--] = x;
}

Those are things that I (or someone else) marked as having been changed to account for overflow. Some of that is from Boom.

I keep going back to that number 65514. That screams 16-bit overflow to me. The > 2048 thing should not be an issue anymore, cause the Wiggle code makes ALL walls as suseptible as the 2048 limit used to, cause it rescales to be as close to overflow (without overflowing) as you can get. But, with your error, it appears to have gone "16-bit negative".

Please let me know what you come up with.