cl_visedicts: are duplicate entries OK/expected?

Discuss programming topics for the various GPL'd game engine sources.
Johnny Law
Posts: 22
Joined: Mon Apr 28, 2014 8:34 pm
Location: San Carlos, CA
Contact:

cl_visedicts: are duplicate entries OK/expected?

Post by Johnny Law »

As a learning experience (and out of curiosity) I've been looking into some of the known issues of the reQuiem engine. Some I can track down just from general C debugging, but in other cases I need to pick up more understanding of the Quake data structures and how they're used.

In one case, I see a linked list that is getting corrupted because it is processing cl_visedicts without taking into account that there could be multiple elements of that array that point to the same object. What I don't yet know is whether:

a) it is bad/unexpected for cl_visedicts to have duplicate entries

or

b) duplicates are OK and the linked-list manipulation code should take that possibility into account.


Can someone give me a push in the right direction?

(If you want to climb further back along my debugging logic, it's this issue: https://github.com/SpiritQuaddicted/reQuiem/issues/9 )
Spike
Posts: 2914
Joined: Fri Nov 05, 2004 3:12 am
Location: UK
Contact:

Re: cl_visedicts: are duplicate entries OK/expected?

Post by Spike »

however you look at it, dupes are pointless and inefficient. the only way that it might actually do anything is if .alpha isn't 1, but there's better ways to handle that.
if you have multiple scenes being drawn (ie: recursing through water leafs) then you should probably remove any entities already on the list just so you don't draw them twice or whatever.

I can't think of any specific places where dupes would actually be crashy, but then its been a long time since I seriously looked at NQ code. QW generates the data in-place, instead of using pointers to some other random bits of memory, its cleaner that way.
Johnny Law
Posts: 22
Joined: Mon Apr 28, 2014 8:34 pm
Location: San Carlos, CA
Contact:

Re: cl_visedicts: are duplicate entries OK/expected?

Post by Johnny Law »

The crashing part is the linked list (skychain), which is something new in reQuiem.

reQuiem's handling of cl_visedicts (which allows/causes duplicate entries) doesn't seem to be notably different from other NQ engines, although of course I could be missing something.
Baker
Posts: 3666
Joined: Tue Mar 14, 2006 5:15 am

Re: cl_visedicts: are duplicate entries OK/expected?

Post by Baker »

A thought, right or wrong.

I've looked at the Requiem change log and it appears to have previous builds dated back to 2008. I think prior to the release (early this year?), this served as Spirit's private engine.

*IF* there are previous versions of the source code available, do you have those? You might consider building one a couple of versions ago and see if has the problem and isolate the one that introduced it.

Then focus on the changes/implementation that occurred between those version.
vis_edicts, how I'd describe them wrote: The vis_edicts list are the supposedly visible entities from the server that are not static (like torches or func_illusionary). If I recall correctly, the player himself isn't on this list and if chase_active 1 is enabled, must be manually added.

As best as I know --- 95% certain --- there should never be one entity in vis_edicts list that is listed twice.

The server sends the visible edicts, a complete list of them, as it checks each one in sequence and determines if the player can see them. Because this is a simple "entity #1", "entity #2" for loop duplicates should not be possible.

Having duplicates in the list would render them twice --- which wouldn't cause a problem just slow things down rendering-wise. Duplicates shouldn't cause a crash, but would be in the rendering list twice.
Short version: There shouldn't be duplicate entries on the cl_visedicts, but this doesn't seem likely to cause a crash unless it is a sign of another problem. Then again, Requiem used JoeQuake as the source base as far as I can tell and its particle rendering stuff is touchy.
The night is young. How else can I annoy the world before sunsrise? 8) Inquisitive minds want to know ! And if they don't -- well like that ever has stopped me before ..
Baker
Posts: 3666
Joined: Tue Mar 14, 2006 5:15 am

Re: cl_visedicts: are duplicate entries OK/expected?

Post by Baker »

Ran Requiem (I compiled it and ran debug version). game czg07 + map czg07 + kill = crash.

So I started up FitzQuake085.exe as a dedicated server as such:
c:\quake\fitzquake085.exe -game czg07 +map czg07 -dedicated 1 -window

In FitzQuake085.exe, I typed "status" which showed me my ip address of "192.168.1.4"

I started Requiem and change the gamedir to czg07. I typed "connect 192.168.1.4" to connect to FitzQuake running in a different window.

I type "kill" and Requiem crashed. But the music kept playing. I set a breakpoint in Visual Studio in the beginning of _Host_Frame. Nothing happened [it didn't hit the breakpoint] (which means the _Host_Frame which runs 200 frames per second +/- was no longer happening). So despite still having music playing and a non-responsive client and Visual Studio did not catch an error or hit a breakpoint. FMOD plays the music and was still running.

Now ...

I did this.

c:\quake\fitzquake085.exe -game czg07 +coop 1 +map czg07 -dedicated 1 -window

When I changed the gamedir in Requiem and connected, I could kill myself as many times as I wanted!!!!! No crashes or anything.

Why would coop 1 make a difference? Maybe Requiem does something special when you die? Does it auto-record demos? Do they save on death? Does it zip them up?

Or maybe something in Requiem only executes when a single player game is running locally?

Do you have a particular reason you suspect visedicts is the problem?
The night is young. How else can I annoy the world before sunsrise? 8) Inquisitive minds want to know ! And if they don't -- well like that ever has stopped me before ..
Spirit
Posts: 1065
Joined: Sat Nov 20, 2004 9:00 pm
Contact:

Re: cl_visedicts: are duplicate entries OK/expected?

Post by Spirit »

Baker wrote:*IF* there are previous versions of the source code available, do you have those? You might consider building one a couple of versions ago and see if has the problem and isolate the one that introduced it.
Sadly I only got that one version of the source.
Improve Quaddicted, send me a pull request: https://github.com/SpiritQuaddicted/Quaddicted-reviews
Spike
Posts: 2914
Joined: Fri Nov 05, 2004 3:12 am
Location: UK
Contact:

Re: cl_visedicts: are duplicate entries OK/expected?

Post by Spike »

reQuiemd.exe!R_DrawSkyChainOld() Line 224 + 0xb bytes C
reQuiemd.exe!R_DrawSky() Line 1312 C
reQuiemd.exe!R_RenderScene() Line 1599 C
reQuiemd.exe!R_RenderView() Line 1712 C
reQuiemd.exe!V_RenderView(qboolean force_refdef_recalc=false) Line 1400 C
reQuiemd.exe!SCR_UpdateScreen() Line 2168 + 0x9 bytes C
reQuiemd.exe!SCR_BeginLoadingPlaque(const char * caption=0x01678b4e) Line 1751 C
reQuiemd.exe!Host_Reconnect_f(cmd_source_t src=SRC_COMMAND) Line 658 + 0x9 bytes C
reQuiemd.exe!Cmd_ExecuteString(const char * text=0x0018f5f8, cmd_source_t src=SRC_COMMAND) Line 1023 + 0xc bytes C
reQuiemd.exe!SV_SendReconnect(const char * newmap=0x0018f838) Line 2419 + 0xe bytes C
reQuiemd.exe!SV_SpawnServer(const char * server=0x0018f838, const char * startspot=0x00000000) Line 2513 + 0x9 bytes C
reQuiemd.exe!Host_Restart_f(cmd_source_t src=SRC_COMMAND) Line 624 + 0xb bytes C
or in other words, your server code shuts down everything, purges the world model, displays the loading screen, and draws the world when it has already been destroyed...
make sure cl.worldmodel->needload isn't set, and if it is, don't draw it?

using coop instead works because it isn't executing the restart console command when you die, and thus isn't executing console commands at the standard (safe) time.
Johnny Law
Posts: 22
Joined: Mon Apr 28, 2014 8:34 pm
Location: San Carlos, CA
Contact:

Re: cl_visedicts: are duplicate entries OK/expected?

Post by Johnny Law »

Baker wrote: Do you have a particular reason you suspect visedicts is the problem?
Details at https://github.com/SpiritQuaddicted/reQuiem/issues/9

Duplicate cl_visedicts entries can cause a loop in skychain. (This part isn't speculation, it's something I observed.)
Last edited by Johnny Law on Wed Jul 09, 2014 3:12 pm, edited 2 times in total.
Johnny Law
Posts: 22
Joined: Mon Apr 28, 2014 8:34 pm
Location: San Carlos, CA
Contact:

Re: cl_visedicts: are duplicate entries OK/expected?

Post by Johnny Law »

Spike wrote:
make sure cl.worldmodel->needload isn't set, and if it is, don't draw it?
Hmm yep, that sounds like it could keep this particular case from happening, but it seems like there's a root cause that should be addressed. There are lots of spots that will call SCR_UpdateScreen, which will (down the callstack) increment r_framecount and add things into cl_visedicts. If multiple of those calls happen between invocations of CL_RelinkEntities, as is happening here, then it's possible to get dups in cl_visedicts.
ericw
Posts: 92
Joined: Sat Jan 18, 2014 2:11 am

Re: cl_visedicts: are duplicate entries OK/expected?

Post by ericw »

just came across this comment from mh which sounds related:
http://forums.inside3d.com/viewtopic.ph ... 507#p48507
Johnny Law
Posts: 22
Joined: Mon Apr 28, 2014 8:34 pm
Location: San Carlos, CA
Contact:

Re: cl_visedicts: are duplicate entries OK/expected?

Post by Johnny Law »

Welp! That's confirmation, thanks.

Guess the most prudent fix for reQuiem would be to change the skychain management so that it doesn't implode when there are duplicate cl_visedicts elements.
Baker
Posts: 3666
Joined: Tue Mar 14, 2006 5:15 am

Re: cl_visedicts: are duplicate entries OK/expected?

Post by Baker »

I was able to stop the crash in Requiem ...

There is a great function in Requiem:

Code: Select all

void CL_LinkEntity(entity_t *ent)
{
	extern qboolean	scr_drawloading; // Baker added
	if (scr_drawloading)  // Baker added
		return;   // Baker added

	if (cl_numvisedicts < MAX_VISEDICTS)
	{
		cl_visedicts[cl_numvisedicts++] = ent;
	}
}
1. Find all locations of cl_numvisedicts++

2. The code will look like this:

Code: Select all

	cl_visedicts[cl_numvisedicts] = ent;
	cl_numvisedicts++;
OR

Code: Select all

cl_visedicts[cl_numvisedicts++] = ent;
OR

Code: Select all

cl_visedicts[cl_numvisedicts++] = pent;
And replace it with the equivalent of this:

Code: Select all

#if 1
		CL_LinkEntity (ent); // Runs this function, which will check scr_drawloading.
#else
		if (cl_numvisedicts < MAX_VISEDICTS)
		{
			cl_visedicts[cl_numvisedicts] = ent;
			cl_numvisedicts++;
		}
#endif
Will stop the crash.
The night is young. How else can I annoy the world before sunsrise? 8) Inquisitive minds want to know ! And if they don't -- well like that ever has stopped me before ..
Baker
Posts: 3666
Joined: Tue Mar 14, 2006 5:15 am

Re: cl_visedicts: are duplicate entries OK/expected?

Post by Baker »

Here is why I believe Requiem is vulnerable to this problem:

1) FitzQuake runs particles once per frame.
2) JoeQuake runs particles when rendering.

FitzQuake-ish engine:

Code: Select all

void _Host_Frame (float time)
{

...
	// update video
	if (host_speeds.value)
		time1 = Sys_DoubleTime ();

	{
		SCR_UpdateScreen ();
		if (!sv.frozen)
			CL_RunParticles (); //johnfitz -- seperated from rendering
	}

...
JoeQuake (Requiem inherited ...) does it during the rendering process.

SCR_Update --- > R_RenderView --> R_DrawParticles --> Classic_DrawParticles|QMB_DrawParticles (those 2 functions spawns particles)

This function runs particle physics:

Code: Select all

void Classic_DrawParticles (void)
{
	particle_t		*p, *kill;
	int				i, j;
	float			grav, time1, time2, time3, dvel, frametime;
	unsigned char	*at, theAlpha;
	vec3_t			up, right;
	float			dist, scale, r_partscale;

	if (!active_particles)
		return;

	r_partscale = 0.004 * tan(r_refdef.fov_x * (M_PI / 180) * 0.5f);

	GL_Bind (particletexture);

	glEnable (GL_BLEND);
	if (!gl_solidparticles.value)
		glDepthMask (GL_FALSE);
	glTexEnvf (GL_TEXTURE_ENV, GL_TEXTURE_ENV_MODE, GL_MODULATE);
	glBegin (GL_TRIANGLES);

	VectorScale (vup, 1.5, up);
	VectorScale (vright, 1.5, right);

	frametime = fabs(cl.ctime - cl.oldtime);
	if (cl.paused)		// joe: pace from FuhQuake
		frametime = 0;
	time3 = frametime * 15;
	time2 = frametime * 10; // 15;
	time1 = frametime * 5;
	grav = frametime * sv_gravity.value * 0.05;
	dvel = 4 * frametime;

	for ( ; ; )
	{
		kill = active_particles;
		if (kill && PARTICLE_INACTIVE(kill))
		{
			active_particles = kill->next;
			kill->next = free_particles;
			free_particles = kill;
			continue;
		}
		break;
	}

	for (p = active_particles, j=0 ; p ; p = p->next)
	{
		for ( ; ; )
		{
			kill = p->next;
			if (kill && PARTICLE_INACTIVE(kill))
			{
				p->next = kill->next;
				kill->next = free_particles;
				free_particles = kill;
				continue;
			}
			break;
		}

		// JDH (from BJP): Improve sound when many particles
		if (++j % 8192 == 0)
			S_ExtraUpdateTime ();

		// hack a scale up to keep particles from disapearing
		dist = (p->org[0] - r_origin[0])*vpn[0] + (p->org[1] - r_origin[1])*vpn[1] + (p->org[2] - r_origin[2])*vpn[2];
		scale = 1 + dist * r_partscale;

		at = (byte *)&d_8to24table[(int)p->color];
		theAlpha = (p->type == pt_fire) ? 255 * (6 - p->ramp) / 6 : 255;
		glColor4ub (at[0], at[1], at[2], theAlpha);
		glTexCoord2f (0, 0);
		glVertex3fv (p->org);
		glTexCoord2f (1, 0);
		glVertex3f (p->org[0] + up[0]*scale, p->org[1] + up[1]*scale, p->org[2] + up[2]*scale);
		glTexCoord2f (0, 1);
		glVertex3f (p->org[0] + right[0]*scale, p->org[1] + right[1]*scale, p->org[2] + right[2]*scale);
		p->org[0] += p->vel[0] * frametime;
		p->org[1] += p->vel[1] * frametime;
		p->org[2] += p->vel[2] * frametime;

		switch (p->type)
		{
		case pt_static:
			break;

		case pt_fire:
			p->ramp += time1;
			if (p->ramp >= 6)
				p->endtime = -1;
			else
				p->color = ramp3[(int)p->ramp];
			p->vel[2] += grav;
			break;

		case pt_explode:
			p->ramp += time2;
			if (p->ramp >= 8)
				p->endtime = -1;
			else
				p->color = ramp1[(int)p->ramp];
			for (i=0 ; i<3 ; i++)
				p->vel[i] += p->vel[i]*dvel;
			p->vel[2] -= grav * 30;
			break;

		case pt_explode2:
			p->ramp += time3;
			if (p->ramp >= 8)
				p->endtime = -1;
			else
				p->color = ramp2[(int)p->ramp];
			for (i=0 ; i<3 ; i++)
				p->vel[i] -= p->vel[i]*frametime;
			p->vel[2] -= grav * 30;
			break;

		case pt_blob:
			for (i=0 ; i<3 ; i++)
				p->vel[i] += p->vel[i]*dvel;
			p->vel[2] -= grav;
			break;

		case pt_blob2:
			for (i=0 ; i<2 ; i++)
				p->vel[i] -= p->vel[i]*dvel;
			p->vel[2] -= grav;
			break;

		case pt_grav:
		case pt_slowgrav:
			p->vel[2] -= grav;
			break;

#ifndef _WIN32
        default:
            break;      // shut up compiler warnings
#endif
		}
	}

	glEnd ();
	glDisable (GL_BLEND);
	glDepthMask (GL_TRUE);
	glTexEnvf (GL_TEXTURE_ENV, GL_TEXTURE_ENV_MODE, GL_REPLACE);
	glColor3f (1, 1, 1);
}
The night is young. How else can I annoy the world before sunsrise? 8) Inquisitive minds want to know ! And if they don't -- well like that ever has stopped me before ..
Johnny Law
Posts: 22
Joined: Mon Apr 28, 2014 8:34 pm
Location: San Carlos, CA
Contact:

Re: cl_visedicts: are duplicate entries OK/expected?

Post by Johnny Law »

Thanks for poking around at this some more. :)

So, looking at the patch above:

Code: Select all

void CL_LinkEntity(entity_t *ent)
{
	extern qboolean scr_drawloading; // Baker added
	if (scr_drawloading)  // Baker added
		return;   // Baker added

	if (cl_numvisedicts < MAX_VISEDICTS)
	{
		cl_visedicts[cl_numvisedicts++] = ent;
	}
}
That will stop this particular lockup because it will avoid adding to cl_visedicts in the case where SCR_UpdateScreen is called from SCR_BeginLoadingPlaque.

I'm still concerned about all the other spots where SCR_UpdateScreen is called though... seems like those can also cause dups. So I'm wondering if there is a more general solution. (Or something that I'm missing about why a more general solution isn't needed.)
Baker
Posts: 3666
Joined: Tue Mar 14, 2006 5:15 am

Re: cl_visedicts: are duplicate entries OK/expected?

Post by Baker »

Johnny Law wrote:I'm still concerned about all the other spots where SCR_UpdateScreen is called though... seems like those can also cause dups. So I'm wondering if there is a more general solution. (Or something that I'm missing about why a more general solution isn't needed.)
Have you figured out why it crashes in particular? Sounds like you know where it is crashing, but what is it doing when it crashes -- and why?

Does the engine hardly ever crash during gameplay? Or does it crash with some frequency?
The night is young. How else can I annoy the world before sunsrise? 8) Inquisitive minds want to know ! And if they don't -- well like that ever has stopped me before ..
Post Reply