Interesting QC Bugs

[Dutch]

A little side project of mine right now is digging through every line of id Software's code in progs 1.06 and cleaning up, re-organizing, and fixing any bugs. This is in the interest of two things:

1) Fully understand EVERY aspect of the gamecode in EVERY regard
2) Provide a clean, bug-free, OCD-level organized package of source code to start projects from.

All deviations from the original code, including improvements, alterations, and bugfixes, are being logged.

Anyways, I've found a few bugs that to my knowledge have not been documented anywhere, so I thought I'd share my findings and my method of correction.

================================================== ===

Interesting Bug #1: Projectiles pass through Shambler

Go start up Quake (no mods, just id1), do impulse 9, equip the rocket launcher, and find a Shambler. You may want to activate god mode. Start shooting him. Ever notice how every now and then, seemingly at random, a rocket (or nail) will pass right through him? I actually never noticed this till tonight. I tested this on QuakeSpasm and ProQuake with the same results, so I deduced this wasn't strictly an engine thing having to do with entity size or whatever.

Turns out that in the shambler's QC file, the bolt of lightning model that spawns overhead when he's casting his magic attack is assigned his .owner. This is in the interest of tracking the bolt across the Shambler's animation frame functions and changing the bolt's frame as well. After a lot of testing, I discovered that this was the cause of the projectiles intermitently passing through him. I created a new .entity bolt and assigned the model to it. So instead of the bolt being the shambler's 'self.owner', it is now his 'self.bolt'. This appears to have fixed the issue.

I'm almost certain that .owner is referenced somewhere in the engine C code. Too lazy to dig through the engine source. Maybe one of you engine guys knows the connection.

[MadGypsy]

Good stuff, Dutch! I like reading stuff like this. Keep it up.

[Spike]

the engine later reuses the 's_light.mdl' entity for your rocket (about 0.5 secs later), and fails to clear self.owner on the shambler.
(for people that don't know, .origin is special in that proper use of it causes projectiles to NOT hit the owning player's bbox as soon as its fired.)
so when the entity gets reused as a projectile, the code basically treats the shambler as though it were a projectile of the rocket. yes backwards, but it has to work both ways. this causes your rocket to pass straight through the shambler.
so yeah, the proper fix is to clear the shambler's .owner field once the bolt effect is removed such that it can't later refer to entities that get spawned as something else. failing that, just using a different field will have a similar result (and more straight forward). the entity reference will still linger, but oh well.

[Dutch]

(for people that don't know, .origin is special in that proper use of it causes projectiles to NOT hit the owning player's bbox as soon as its fired.)

When you said .origin here, did you mean .owner? I have noticed that for fired rockets and grenades to contact the player who fired them, their ownership had to be assigned to world. I usually code a recursive think function for fired projectiles that will assign ownership to the world a small amount of time after a player fires them, to ensure that they won't explode on them upon launch. I then store the player as the projectile's old owner via a new .entity field, that way the projectile can still be tied to the player who launched them (for gaining frags and proper obituary messages and what not). I went to this trouble because I coded teleporters to teleport rockets and grenades, and wanted them to hit the player if he was able to get in front of them after they teleported (like on the Start map). That, and rocket jumping with grenades is badass.

Thanks for the explanation, I'm glad I was on the right track with that. I might see about clearing the Shambler's .owner field, that way I can keep lingering references to a minimum. But with today's RAM levels, it probably doesn't matter...

@gypsy

Thanks bro! I'll have a few more to post up before too long. Actually, I got one right now (see next post)...

[Dutch]

Interesting Bug #2: Spikeball Avelocity, properties, and movement code

The original QC assigns an avelocity to the spikeball ("misc_teleporttrain"), the ball that hovers around the End map that the player must use to telefrag Shub-Niggurath. However, you may have noticed that it does not spin at all as it moves about the map. This is because it's movetype is set to MOVETYPE_PUSH, which is reserved for BSP bmodel entities. In fact, this would normally cause a game crash if the player or monster is able to contact it, if it didn't have .solid of SOLID_NOT.

To achieve proper avelocity, it needs to be a MOVETYPE_FLY and SOLID_TRIGGER. However, upon changing this, it moves at blinding speeds, constantly hopping from one path_corner to the next. Why? Well as it turns out, the Spikeball uses the same think function as bmodel trains. In short, a train will measure the distance between itself and the next path_corner it targets. Using this distance and its speed, a time is calculated for it to travel along this vector. This calculation is actually very accurate. This is done in SUB_CalcMove, which is called on in the train's think function, train_next. Once the time has elapsed, it is at the next path_corner, and its next think function will set its origin to the path_corner's origin, at which point the think function will search for the next target path_corner, and the cycle continues. The nextthink field (time until the next think function is fired) is in relation the train's .ltime, or local time. The ltime field only works for BSP entities, not MDL models (like the Spikeball). This means the ltime is monitored in the engine somewhere, I would imagine.

So how to correct this? Think functions and SUB_CalcMove need to check if the entity's classname is "misc_teleporttrain", and set its nextthink to global time.

To recap: set the Spikeball's movetype to MOVETYPE_FLY and its solid to SOLID_TRIGGER. Maybe set its .touch function to SUB_Null to prevent any odd behavior when it contacts other entities. Run a classname check in SUB_CalcMove and the think functions train_find and train_wait and set nextthink in relation to 'time', not 'self.ltime'.

I'm not sure if there are negative side effects to using global time though. Perhaps a loss in precision over time, resulting in the SpikeBall stuttering to the path_corner's origin when setorigin is called? At any rate, the vanilla MDL model is oblong, making avelocity look ridiculous, so this was a bug I pretty much left alone, but still found entertaining.

EDIT: I discovered that using global time does indeed cause some stuttering behavior as the SpikeBall approaches the path_corner and its origin is reset. Global time is not accurate enough for the nextthink calculation. With this knowledge, my correction now would be to write completely new think functions for the SpikeBall, perhaps a recursive think function that checks how far from the path_corner target it is before updating velocity. Or maybe even a touch function.

[MadGypsy]

@spikeball

This is just off the cuff but, maybe it could be a looser system that does not rely on path corner as a trigger but more as a minimum distance to travel and the point to shoot for.

In other words the global time check could check if the ball is on or past the path entity and if so aim for next path. This could obviously cause problems if the path is too close to a wall but it may be negligible.

I don't know if it's a good idea. It's just an alternate one.

[Spike]

@dutch:
yeah, owner, sorry.

ltime vs time has nothing to do with whether its actually a bsp model or not, for the most part the engine doesn't care (with notable exceptions). What makes the difference is the movetype. movetype_push uses self.ltime, every other movetype has thinks relative to time.

ltime was used so that pushes would completely freeze when blocked, without needing to mess with any other fields. it might have been easier to just bump nextthink instead, but that would have accumulated imprecision.

those notable exceptions:
1: SOLID_BSP is required if you want to use the model's own collision data (vanilla: this MUST be a bsp model, dp/fte: this can also be trisoup), otherwise the entity is treated as an aabb.
2: vanilla requires that SOLID_BSP be used only with MOVETYPE_PUSH (which is a little unnecessary as it rules out MOVETYPE_NONE+SOLID_BSP).
3: vanilla assumes that MOVETYPE_PUSH will always be paired with SOLID_BSP (and may break .solid resulting in a crash from the previous exception, and is fixed in any engine that wishes to avoid weird crashes on end.bsp, otherwise you'll get a crash if teletrains pass through anything but shub). In theory an AABB should otherwise be fine too (or non-solid).
4: movetype_push with trisoup collisions will generally fail due to it being unable to detect collisions within the center of the trisoup non-volume, as there are no triangles there to actually collide with. This means fast-moving non-volume pushers are framerate dependant and thus best avoided. This of course does not apply to vanilla at all, as vanilla has no support for trisoup collisions. Also, you don't want to have to deal with animating pushers either.

[MadGypsy]

Something else that could be done as a spin off of my idea is to come from the other end. Instead of at or greater than it could be greater than some position slightly before the target. This would allow you to curve the path between points and basically treat the points as sort of a max bounds for the path to travel in.

As it stands now an entity will immediately switch directions toward the next corner when it reaches the previous corner. This may be good for enemy movement in corridors but it binds open-air flying shit to the same rigid system. A utilization of my idea could have a square pattern of corners make the entity fly in a circle.

Yet again, not necessarily a great idea but, definitely a possible one. Probably even a useful one for future maps. Of course the original straight line system would still have to remain in some form. If you use the examples I sent you as your programming method you could build the curved system right on top op the straight line system by having the curved system inherit from the straight line system. Same way my script builds proper entities.

[Dutch]

Spike Interesting. So the way they did the Spikeball was certainly pretty hacky on id's part. Makes sense they would use local time for push entities...ensures that the train will still fire its think at the proper time even if it got held up smashing a player or whatever. Thanks again for the explanation. Makes this all a lot more interesting learning what the cause of the behavior is.

@gypsy

Yeah I was already brainstorming some ideas for rotational velocity. Definitely something to implement in a project. First goal is to finish the clean-up stage of id's QC. I know about the clean QC project but I'm pretty particular about my code organization. That, and it's a lot more fun to do it myself. We wouldn't even be here on this thread if I just grabbed someone else's code base, ya know?

But I'm going to come up with something to 'correctly' implement the Spikeball, likely along the lines of what you posted. It works now but I don't like hacky bullshit, even if it is original.

[Dutch]

Good news! I found an easy solution to the SpikeBall bug.

We still need to add a classname check to the train think functions for "misc_teleporttrain" and tell them to use global time and not self.ltime. We also need to add this check to SUB_CalcMove. Once we have completely separated any self.ltime references from the Spikeball, we can safely make it a MOVETYPE_NOCLIP. I discovered that the jittery behavior was a result of it being MOVETYPE_FLY and having collisions with the BSP hull, which altered it's velocity vector.

With movetype set to noclip and only using global time, the behavior appears the same as the original code, and avelocity now works.

==================

Side note: I went back to the Shambler bug and got rid of the .bolt field and reverted back to .owner. Except this time, on the animation frame funciton directly after the last frame the bolt model is used, I removed(self.owner) and set self.owner = world. This also corrected the issue and reduced the reference count.

I think when I'm all done with this little project I'm going to post an "Exotic Bug Fix" list with instructions on how to correct them. That buglist over at Inside3d is awesome, but these bugs I've addressed so far I don't recall seeing them on there...

[MadGypsy]

Dutch

I did some research for you. If you are going to fix the entire source how about modernising it as well.

http://forums.insideqc.com/viewtopic.php?f=2&t=5806

That link contains a couple of examples. By observation it would seem like the format is

Class name:type { /* your code*/ }

Use 'this' instead of 'self' in a class

To use existing shit like centerprint prepend your line with 'virtual' and shove the reference in curly brackets

Finally to call your class use

Yourclass = spawn(); // this is so cute

Easypeasy, bro. I like QC classes.

[Dutch]

Sweet! I'll try it out tonight! Thanks bro.

[Dutch]

Not so much a bug, but an interesting point. The engine handles the built-in particle function entirely differently with a certain color parameter, namely 255. It produces the exact same effect as TE_EXPLOSION. Example:

particle(self.origin, '0 0 0', 75, 255);

yields the same result as

WriteByte(MSG_BROADCAST, SVC_TEMPENTITY);
WriteByte(MSG_BROADCAST, TE_EXPLOSION);
WriteCoord(MSG_BROADCAST, self.origin_x);
WriteCoord(MSG_BROADCAST, self.origin_y);
WriteCoord(MSG_BROADCAST, self.origin_z);

Almost all explosions send a broadcast, whereas the explosion boxes found in the base maps simply call up the particle function. Changing the color input to 254 or any other value (255 is max) results in entirely different particle behavior. However, the engine injects the explosion sound for the broadcast, whereas (not surprisingly) the particle function is silent.

I wonder if this has a noticeable difference for clients over a network?

=======================

QC classes looks to be promising, gonna be a learning curve. I'll start playing around with it. I love FTEQCC by the way. I never realized how robust it is in what it will accept.

[Dutch]

A little update on my effort to 'modernize' the stock QC. So far, aside from ternary statements, it's still pretty old-school in form and function. Implementing more advanced techniques will be a goal after thoroughly organizing and trimming the code. Results thus far (about 1/2 way done):

1) Reduced the compiled PROGS.DAT by 59kb (originally 404, now 345).
2) Eliminated around 7 or 8 fields (such as .waitmin, .healamount, and the like). I forget the actual number. Let's assume 7. Average quake map probably has 400-700 entities. That's anywhere from 2800-4900 references removed.
3) Re-structured SEVERAL recurring bits of code into delegated subroutines, making it a lot easier to read, mod, and track problems.

This has all been achieved without drastically altering gameplay. Aside from obvious bugfixes, alterations are kept to a bare minimum. Sometimes it's unaviodable...I'm not going to suffer lengthy, cluttered code for a single classname check. Example: the check attack system for monsters. When you really break it all down and get to the bare bones of each monster's check attack, better than half of them share identical properties except for a very very small randomized percentage of launching an attack (we're talking 0.05 difference). I simply averaged the differences and rolled them into one short, concise function that calls up more functions if needed based on the set of attacks any given monster has. Example:

if (self.th_melee && self.th_ranged)
return CheckEitherState();
else if (!self.th_melee && self.th_ranged)
return CheckRangedState();
else
return CheckMeleeState();

Monsters either have a melee attack, a ranged attack, or both. Obviously there are special cases that need to be handled differently that don't fit this paradigm (the scrag, for instance, since it has an additional check for strafing). But the amount of lines of code I trimmed using this method is absolutely substantial, and gameplay deviation is hardly noticeable (a beginner or a casual gamer won't notice at all, and seasoned veterans may not either).

EDIT: I understand id's original check attack system utilized one function for about half of the monsters already...but it was done in a way that the single function checked against several unnecessary scenarios for each monster, trying to rope them all in in the interest of maintaining a few very subtle differences.

[MadGypsy]

If(( value = CheckRangedState() ) != somethingLikeNullOrFalse )
    return value;

return CheckMeleeState();

Why really check anything? Do the most common and if null/false/whatever return the other. Then in the callee check null/false/whatever on the return.