Physics/Lerp/Timing Bug Introduced in 0.85.5

[KillPixelGames]

projectile_offset.mp4

Projectiles spawn closer to the camera the first few times they're fired then correct themselves. Tested their TTL and they indeed do change position.

Bug not present in WinQuake, FtizQuake, Quakespam versions prior to 0.85.5, DP or FTE.

I've been going through the changes between QS 85.4 and 85.5 and can't find anything obvious to me (but I'm not a programmer).

A test was also created with vanilla quake progs and the bug is present.

EDIT: This bug is very difficult to see with vanilla quake because the rocket/nail projectile models are much shorter than the laser model featured above. Hiding the viewmodel and/or changing the projectile model or left/right offset helps make it more apparent.

[andrey-budko]

Not sure if it's a bug, but this change seems to fix the described issue.

diff --git a/Quake/sv_phys.c b/Quake/sv_phys.c
index a9fb3f31..fd42d47a 100644
--- a/Quake/sv_phys.c
+++ b/Quake/sv_phys.c
@@ -1190,8 +1190,8 @@ void SV_Physics (void)
 	else
 	  entity_cap = sv.num_edicts;
 
-	//for (i=0 ; i<sv.num_edicts ; i++, ent = NEXT_EDICT(ent))
-	for (i=0 ; i<entity_cap ; i++, ent = NEXT_EDICT(ent))
+	for (i=0 ; i<sv.num_edicts ; i++, ent = NEXT_EDICT(ent))
+	//for (i=0 ; i<entity_cap ; i++, ent = NEXT_EDICT(ent))
 	{
 		if (ent->free)
 			continue;

Tested with bolt.mdl instead of spike.mdl

Added some logging to investigate:

diff --git a/Quake/sv_phys.c b/Quake/sv_phys.c
index a9fb3f31..1102464c 100644
--- a/Quake/sv_phys.c
+++ b/Quake/sv_phys.c
@@ -1193,6 +1193,8 @@ void SV_Physics (void)
 	//for (i=0 ; i<sv.num_edicts ; i++, ent = NEXT_EDICT(ent))
 	for (i=0 ; i<entity_cap ; i++, ent = NEXT_EDICT(ent))
 	{
+		if(entity_cap != sv.num_edicts)
+			Con_Printf ("i:%d, entity_cap:%d sv.num_edicts:%d\n", i, entity_cap, sv.num_edicts);
 		if (ent->free)
 			continue;
 

Output:
i:2, entity_cap:85 sv.num_edicts:86
i:3, entity_cap:85 sv.num_edicts:86
i:4, entity_cap:85 sv.num_edicts:86
i:5, entity_cap:85 sv.num_edicts:86
i:6, entity_cap:85 sv.num_edicts:86
i:7, entity_cap:85 sv.num_edicts:86

[alexey-lysiuk]

Looks like a bug to me. In the given case, when an entity is spawned during SV_Physics_Client() call, its processing won't happen on the current frame. In other words, a new entity will be updated properly on the next frame only.
With sv_freezenonclients CVAR set to zero, loop should query actual value of sv.num_edicts. Potentially, other SV_Physics_...() functions may add an entity as well.

[sezero]

@ericwa : Please have a look. (CC: @andrei-drexler )

[andrey-budko]

Looks like I still have this issue with host_maxfps set to 240 (which is my default value because of Ironwail)

[sezero]

Looks like I still have this issue with host_maxfps set to 240 (which is my default value because of Ironwail)

Not relevant to this particular ticket, no?

And it's a non-bug, i.e. don't do that ?

[sezero]

Not sure if it's a bug, but this change seems to fix the described issue.

Looks like a bug to me. In the given case, when an entity is spawned during SV_Physics_Client() call, its processing won't happen on the current frame. In other words, a new entity will be updated properly on the next frame only. With sv_freezenonclients CVAR set to zero, loop should query actual value of sv.num_edicts. Potentially, other SV_Physics_...() functions may add an entity as well.

Looks like the issue surfaced with Steve's sv_freezenonclients patch added in qs-0.85.5: 44ecfb1
Sent him an e-mail (with @ericwa CC'ed)

[vsonnier]

Looks like a bug to me. In the given case, when an entity is spawned during SV_Physics_Client() call, its processing won't happen on the current frame. In other words, a new entity will be updated properly on the next frame only.

Well, I don't think the present patch will solve anything, except if there is indeed no free edict left, and there is a need to create one more from scratch increasing num_edicts in that case.

For all the other cases, we are still scanning edicts from 0 .. num_edicts in one pass :

for (i=0 ; i<sv.num_edicts ; i++, ent = NEXT_EDICT(ent))
{    
    Process_Edict(i);
}

So if Process_Edict(i) has side effects on the rest of the list of edicts, whatever they are (freeing, allocating, changing state) all changes happening for edicts f in 0 < f < i won't be seen until the next frame, for each i.
So it is pure luck that f are taken into account in the same loop/frame anyway.

And the situation is worse for SV_Push_Move where you have a double loop over edicts (pushers x pushable entities)

Unless I'm completly wrong here, this is just the way things are and the above patch only solve a fringe case.

[sezero]

One hacky solution I can think of, without reverting the sv_freezenonclients patch
is something like this: bug-124.patch (inlined below)
Is it correct?? Does it help at all??

diff --git a/Quake/sv_phys.c b/Quake/sv_phys.c
index a9fb3f3..538253f 100644
--- a/Quake/sv_phys.c
+++ b/Quake/sv_phys.c
@@ -1169,7 +1169,7 @@ SV_Physics
 void SV_Physics (void)
 {
 	int	i;
-	int	entity_cap; // For sv_freezenonclients 
+	int	entity_cap, *p_entitycap; // For sv_freezenonclients
 	edict_t	*ent;
 
 // let the progs know that a new frame has started
@@ -1183,15 +1183,12 @@ void SV_Physics (void)
 //
 // treat each object in turn
 //
+	entity_cap = svs.maxclients + 1;
+	p_entitycap = (sv_freezenonclients.value) ? &entity_cap // Only run physics on clients and the world
+						  : &sv.num_edicts;
 	ent = sv.edicts;
 
-	if (sv_freezenonclients.value)
-	  entity_cap = svs.maxclients + 1; // Only run physics on clients and the world
-	else
-	  entity_cap = sv.num_edicts;
-
-	//for (i=0 ; i<sv.num_edicts ; i++, ent = NEXT_EDICT(ent))
-	for (i=0 ; i<entity_cap ; i++, ent = NEXT_EDICT(ent))
+	for (i=0 ; i<*p_entitycap ; i++, ent = NEXT_EDICT(ent))
 	{
 		if (ent->free)
 			continue;

[vsonnier]

@sezero Thats pretty much it I think. With this in the case a a sudden surge of new edicts exceeding the free pool (projectiles, particles ?) those will be taken into account in the current frame. That is only likely to occur at the begining of a level though, where free edicts are few... Don't know yet if I'll take it for vk.

[sezero]

A simplified / better-readable patch: bug-124.patch (also inlined below.)

If there are no objections (@KillPixelGames, @andrey-budko, @vsonnier, @alexey-lysiuk) I'd like to apply it later today.

diff --git a/Quake/sv_phys.c b/Quake/sv_phys.c
index a9fb3f3..a59d657 100644
--- a/Quake/sv_phys.c
+++ b/Quake/sv_phys.c
@@ -1170,6 +1170,7 @@ void SV_Physics (void)
 {
 	int	i;
 	int	entity_cap; // For sv_freezenonclients 
+	int	*pentitycap;
 	edict_t	*ent;
 
 // let the progs know that a new frame has started
@@ -1186,12 +1187,14 @@ void SV_Physics (void)
 	ent = sv.edicts;
 
 	if (sv_freezenonclients.value)
+	{
 	  entity_cap = svs.maxclients + 1; // Only run physics on clients and the world
+	  pentitycap = &entity_cap;
+	}
 	else
-	  entity_cap = sv.num_edicts;
+	  pentitycap = &sv.num_edicts;
 
-	//for (i=0 ; i<sv.num_edicts ; i++, ent = NEXT_EDICT(ent))
-	for (i=0 ; i<entity_cap ; i++, ent = NEXT_EDICT(ent))
+	for (i = 0; i < *pentitycap; ++i, ent = NEXT_EDICT(ent))
 	{
 		if (ent->free)
 			continue;

[alexey-lysiuk]

Looks good to me.

[sezero]

OK, patch applied and pushed as commit 56e21e3

@KillPixelGames : If you can't build yourself, tell me what operating system you are on and I'll try and provide you with a binary from latest git.