What does the code that makes the player detach when they jump look like? Can you copy that into W_CheckNoAmmo? 

Tbh I don't know why jump is detaching you from the chain in general. However, I realized now that you somehow must get into the firing function once again after you ran out of hook ammo.

This means something really needs to happen in W_CheckNoAmmo. You have to get back into the hook firing state with zero ammo and there you need a condition for exactly this situation, just detaching you from the hook and that's it. 

I think finding the jump code would be telling. My hope is that there's a function called something like HookDetatch() which is what you need to add to W_CheckNoAmmo, but it depends how the person writing the original code did it. They might have chosen to just add the code that detaches the player "inline" - copy-pasting it rather than splitting it out into a useful function.

I would start the search for the code in PlayerJump in client.qc. You might want to have PlayerJump from the original ID1 source available for reference, so that any added lines of code will stand out. If that doesn't yield anything, it might be that some of piece of code is checking for the jump button directly. I'd search for references to "button2" as this is the field which contains the jump button status - but you'd have to search the entire code for it. 

How about a hackish approach: Simply let the player jump via attack button if hook ammo is zero and player is still attached to the hook.

Could be as simple as
if (self.button0) { PlayerJump (); }

Basically that's what is supposed to happen right now, but at least you would keep using the attack key. Might this work? 

It seems the idea was a good one. I was able to find the jump function in client.qc and got inspired.

The working code part for weapons.qc looks as follows:

void () W_FireChain;

float () W_CheckNoAmmo =
{
if (self.currentammo > DROIDMODE_GRAP)
{
return (TRUE);
}
if ((self.grap_out == GRAP_STUCK) && (self.button0))
{
W_FireChain ();
W_ToggleDroid ();
}
else if (self.weapon != IT_AXE)
{
self.weapon = W_BestWeapon ();
}
W_SetCurrentAmmo ();
return (FALSE);
};

Works like a charm. Thanks to everybody who gave useful hints! 

I have posted my finished Shrak patch over at the QuakeOne forums, in case anybody is interested. Source files are included. 

Well done Nightfright!

Would you be so kind to publish a working shark.qc? 

If you mean the source code, it's included with the patch in a separate dir. I can upload that separately somewhere (if I knew where). 

See the screenshot thread. 

Comes with documentation about applied changes:
Download (183 KB) 

FYI, I created a news thread for your patch. 

Good job!I wonder how that Gorok enfolds. 

It seems there is another problem in the Shrak code, something I didn't know about until it was pointed out to me in the QuakeOne forums.

If you use the nextweapon or lastweapon keys really fast, the game will crash. It will already happen in the start map when you just have the default weapon. Apparently won't happen if you have all weapons plus the full utility vest with all three items.

Example for the error:
EQ_F 4226 (?) 241 (FL_JUMPRELEASED)4096.0 4226(?)

weapons.qc: CycleWeaponCommand
weapons.qc: ImpulseCommands
weapons.qc: W_WeaponFrame
client.qc: PlayerPostThink

(No Function)
Runaway Loop Error
Host_Error: Program Error 

It was something in ImpulseCommands, as I suspected right away. An abort/delay command was placed too far down in the code. Placing it in the beginning prevents the crashes from happening. 

Still, it feels like a workaround since there is a delay while switching weapons while original Quake does not have it. If you remove the delay entirely or reduce the delay too much (below self.dlayTimer = time+0.1 in client.qc/PlayerPostThink), crashes will reappear again when switching weapons too fast. 

I never use switch weapons, only the number keys.

A big Hurah for the four stars encountered: DOE.qc - SOA.qc - Malice.qc - Shrak.qc !

Nightfright, you rock! 

The Friend Maker (friend.qc) used to shoot a cloud of yellow pixels, but when using this patch, it does not appear. Couldn't find it anywhere in the code. 

It seems instead of
missile.effects = EF_BRIGHTFIELD;
I had to put this in friend.qc:
missile.effects = 1;

No idea why it doesn't work with the actual name, but main thing is it works. 

I believe I have tracked down the origin of the crashes when switching Shrak weapons.

Whenever W_ChangeWeapon or one of the two weapon cycling (impulse 10/12) functions is called, it's flashing the weapon in the HUD, something like flashWeapon (fl) or flashWeapon (self.weapon). If I comment out this line, crashes won't happen any more regardless of mousewheel scrolling speed.

The function is as follows:

void () flashWeaponBack =
{
if (!(self.owner.items & self.items))
{
self.owner.items = (self.owner.items + self.items);
}
remove (self);
};

void (float fl) flashWeapon =
{
local entity e;

e = spawn ();
e.owner = self;
e.items = fl;
e.think = flashWeaponBack;
e.nextthink = (time + 0.1);
if (self.items & fl)
self.items = (self.items - fl);
};

Can anybody see what could be wrong with this? 

OK, I think I see the shape of the problem if not the exact fix, brace yourself...

The crash you posted a little while back contains the error message indicating there's an infinite loop going on. CycleWeaponCommand is built around a potentially infinite loop:

while (1)
{
...
}

The only escape from the loop is the return statement near the bottom

if ((self.items & self.weapon) && (am == DROIDMODE_GRAP))
{
flashWeapon (self.weapon);
W_SetCurrentAmmo ();
return;
}

The first run through the loop looks at your current weapon and "suggests" changing to the next weapon in the game after that. This final bit of code checks whether you have the suggested weapon and ammo to use it; if you do the code changes to it and exits the loop. If not, the next run through the loop starts from the suggested weapon, and changes the suggestion to the weapon after that.

In regular Quake, there is an implicit guarantee that this loop will terminate: the player always has the axe, and the axe is always a valid weapon to select. The loop will suggest the axe after at most 8 attempts, the axe is always possessed by the player, and there's never an ammo requirement for wielding it.

In the Shrak code, the weapon of last resort turns out to be in slot 2 (IT_SHOTGUN). This is the only weapon which doesn't have an ammo requirement. This would be OK ordinarily, and it's only when we consider the effect of flashWeapon that we get to the problem.

flashWeapon makes the icons flash on the hud in a rather dramatic fashion. It genuinely removes the weapon from the player's inventory for a fraction of a second, before putting it back. If you've followed what's going on in the description of the loop above, perhaps you can see the problem that arises...

Because the weapon is actually removed from the player inventory, however briefly, there exists a window where CycleWeaponCommand can't select that weapon and has to skip it. If you quickly flash all of the player's weapons at once (maybe cycling weapons rapidly with the scroll-wheel) all of the players weapons can be removed at once. At this point, CycleWeaponCommand has no suitable candidates to select, but it doesn't give up, it just keeps trying all the weapons in loop after loop until the game crashes. 

OK, that's an explanation I understand. I expected some kind of hack here, and tricking the game into "thinking" you are picking up a weapon for the first time during weapon cycling is pretty much that.

One thing to keep in mind: Even the shotgun replacement uses ammo (cells). There is a function which sets its ammo to 1 even if the counter reaches 0, but if you use the Friend Maker which uses the same ammo type, 0 cells are still possible. Shrak in fact does not have any weapon without ammo requirement, even the tools in the axe slot can run out. This might be a problem since the general function structure from original Quake is maintained.

What could be a possible solution besides commenting out that flash function entirely? The way you describe it, only a sufficient delay while switching can bridge that gap. TBH that excessive flashing during switching looks stupid anyway, fast switching would be more important. 

The easiest solution would be to put a counter in that infinite loop, and exit if it reaches a high number like 100, assuming that if you reached that high you have exhausted all options. Not super elegant but I don’t see any downsides. 

What if you put a delay before Impulsecommand() which is higher than the nextthink delay from flashWeapon (time+0.1)? It would slow down weapon cycling, but it would make sure to avoid the situation with no weapon available. That is a poor approach however and I'd rather comment out that useless flashing thing. 

By "ammo requirement", I really meant "ammo requirement within that function" - it's the only weapon which doesn't set the am flag to 1. This in effect means lack of ammo doesn't prevent you switching to the weapon, separate from any ammo requirement for actually attacking.

In theory one approach to fix the bug would be to have a second field called items_borrowed. While an item is being blinked off, you store it in items_borrowed (and remove it from there when you put it back into the regular items field). Then in all the weapon selection code you check if the weapon is in either items OR items_borrowed. Lot of effort to preserve a not particularly useful effect... 

Indeed, it's a useless feature. Since it's only causing issues and contributing nothing to actual gameplay, I decided to just remove the entire function from the code and be done with it. In return you now have super-fast weapon switching without any risk of triggering crashes.

It's interesting that Matt Walsh had apparently been kinda aware of the problem since he implemented a delay timer in ImpulseCommands. However, he placed it in the wrong position within the code and it wasn't working very well in any case. In 1996, available hardware was probably not capable of such fast input which would allow you to send impulse commands in rapid succession, but nowadays mousewheel abuse is entirely possible.

One way or the other, thanks a lot for helping me to figure out what was going on here, guys! I am a bit proud of myself since I was at least able to find the exact location of the error. For someone without any previous QuakeC knowledge, I consider it quite an achievement.