A tsunami of changes and optimizations

[LeGurdah]

I spent like... half a week, half-procrastinating on this, half-working on this. (Or was it a third-quarter of a week?)

Anyway. The title speaks for itself -- I did so many freakin' changes. That was probably worth all the time spent, yea? I made certain to test before finally committing these changes, so there won't be too many errors to come across.

They are all so significant, that people who made addons for this addon specifically might need a warning in advance, so their stuff doesn't break.

=- overall -=

One of the most significant is changing ::Left4Bots' Survivors, Specials, Tanks, and Witches tables into arrays (the entities themselves are tracked instead of needing a userid as their key.) This was mainly for memory purposes. As such, all coding surrounding them have been altered accordingly. (I'm preparing a big change involving the Bots and L4D1Survivors tables in advance.)

Hence, because of that change alone, some stuff could rarely break in the process, of which those could be ones I failed to catch.

=- left4bots.nut -=

Onto the main script; one big change is crunching IsHandledSurvivor, IsHandledBot, and IsHandledL4D1Bot each down into a single line of code! Which might not be much, but because of how they're sometimes used throughout the code, every little bit helps.

Yet another one is altering the "if" conditions', well, ordering of the conditions for "getting specific entities that are nearby" and the like that are iterated over across several functions -- entities' integers and boolean variables are prioritized first before we, say, crunch the numbers and measure distance between an entity. This helps make it faster to actually find the entities those functions are searching for.

GetOtherAliveSurvivors and GetOtherAliveHumanSurvivors are turned into generator functions via the "yield" keyword -- this allows it to return the survivors the code needs right now, instead of putting them into a table and returning that. Thankfully, this didn't require much changes to be made, since they are often used in "foreach" loops, and "foreach" accounts for them. Well, I did that according to this anyway, so who knows. (I might make "list" versions of these that simply return an array with the checked-for survivors, so that way they don't need to worry about using the "resume" keyword.)

A minor optimization is CountOtherStandingSurvivorsWithin incrementing its return value (or smthing, I forgot) via an integer-casted boolean value. And more a nature of "completeness" than anything, but the L4D1Survivors table gets cleared (and the AI removed from the L4D1 survivors) in AddonStop.

=- left4bots_events.nut -=

I changed how SurvivorFlow's values are updated -- originally, OnThinker constantly defined (and redefined on the next time it runs, ad infinitum) the table value of each key on the spot. I did not want to take any chances with this occurring potentially every tick (since that could be constantly throwing away precious memory every frame, which is liable to cause hang-ups,) so I settled on an alternative -- initialize SurvivorFlow on OnPostPlayerSpawn (and the events that do things with SurvivorFlow,) and instead of redefining each keys' table values every time OnThinker runs, update just two values out of three values from SurvivorFlow's keys in it -- inCheckpoint, and flow. isBot is altered accordingly for conditions that succeed in checking if they are a bot. (OnThinker itself has been changed slightly, but that's not as big a deal as the former.)

According to this older Squirrel manual (which is helpful in this context since we're working with an older version,) using the "delete" keyword on a table that doesn't have the key the keyword was used for won't actually do anything -- not even throw up errors. (I'm assuming this is because Squirrel, on the C++ side, already does the check to avoid errors or smthing.) Hence, I commented out the "if" branches that are solely done to check if that key was in the table, before doing so. Testing tells me this will still work as intended.

I also changed _OnGameEvent_playerdeath. Stuff in that event that gets checked exclusively in a debugging scenario are sectioned off into its own "debug" function, and called accordingly. A few branches have been shaved off here, and switched around.

There were instances of GetPlayerUserId being used in events that have user ids as parameters, so I just ditched the former in favor of using those parameters.

=- conclusion -=

There are unquestionably many things that need to be tested on your end -- so please take the time.

If you need a change to be reverted, or an error needs to be patched, I'm not even going to mind -- because at least I helped make these beasts of code more manageable on the game's end.

[4512369781]

I think the table is better, now the data in the table is userid : entity, so if we know the id(like in event), can easily get the entity or delete it form the table, also we can get the id easily when using foreach.

foreach is simpler than for, so if there is no special need, I would choose foreach. For example, in Left4Bots.OnCleaner, whether it is table or array, foreach can be processed, only need to change delete table[id]; to array.remove(id);.

if a key not in the table, delete will throw up [AN ERROR HAS OCCURED [the index 'test' does not exist] error, so need check the key is or not in the table first.

[LeGurdah]

Touche. You've a fair point about the tables.

The only justifications for my favoring arrays that hold entities instead of a table with a User Id key and Entity value is for their memory compactness (as for the question of holding an array of User Ids... I feared that would further complicate the code more than is necessary.) 'Tmakes 'tmore easy for me to directly manage them instead of having to go the extra step and using their User Id to gain access to them. The only times I ever need to use a table with them (or even a table that uses their User Id as a key) is if I want them to each have complex data that need to be tracked by the script, and even then this is because I learned from the Binding of Isaac: Rebirth modding community's user-made guides that 'tis unanimously a better idea to use your own tables for tracking unique data across entities instead of exclusively relying on said entities' "script scopes," to put 'tin this game's equivalent terms, which has the unfortunate problem of being overridable by other mods that also try to make use of that entity's "script scope."

As for the "iterating over and removing things from an array" part from OnCleaner, I had no other choice than to use a "for" loop and manually remove the currently-iterated (and invalidated) index in lieu of "foreach" because that could mean having to call the array's find function, which executes a binary search for the thing-to-be-removed's index (because the array's remove function removes the specified index's item, and 'trequires a valid index to be passed as a parameter in order to do so,) which I then would need to check if the resulting index is not null and then remove that, all of which could... 'er. Be a tiny bit overkill.

I willingly leave almost all instances of "foreach" intact due to the advantages of 'ts simplicity (as you brought up,) only really breaking out the "for" loop if I need to do something more than directly interact with the entities within them, such as what I did with iterating over the Witches array in that one "finding the nearest enemy" function if a "return" entity was not found. (I used two conditions in that "for" loop, which wasn't out of the ordinary for me because I also did that in a separate and unrelated game mod as well to avoid using an "if" condition to solely break said "for" loop.)

Your last bit about a Squirrel error being thrown up because of the absence of a slot to delete in a table is... strange. Granted, I never ended up coming across that while testing my changes. Perhaps this could be easier to catch in a stress test?

Left 4 Dead 2 uses Squirrel 3.0.4 (according to the version constant anyway,) and in the Virtual Machine section of the Squirrel manual I linked (which also concerns version 3.0, and the VM functions themselves being implemented on the C side,) specifically the function _sqdeleteslot, the latter's description is as follows: "pops a key from the stack and delete the slot indexed by it from the table at position idx in the stack, if the slot does not exits[sic] nothing happens."

It claims nothing will happen if the slot does not exist in the table, which I took it to mean that _sqdeleteslot automatically checks for the presence of a slot in the table, and does nothing (not even throw errors) if it isn't actually there.

I'm certain the "delete" keyword calls this VM function, so for the VM to still throw up an error, contrary to _sqdeleteslot's description, due to the predetermined absence of a slot... is still strange. That is, unless the function itself was deemed too inefficient for Valve and they instead used a different function in its place or smthing, which could potentially explain the error existing at all. (But then that would raise further questions, which I will not get into in the name of brevity.)

[smilz0]

Sorry for the delay. This is really a lot of changes it will take some time. For now i can only say that i also remember the delete with a nonexistent key throwing errors but i might be wrong. I think this can easily be confirmed by looking at the squirrel's source code. It can also be useful to see how the tables and arrays are implemented. I used to use arrays thinking that searching an integer in an ordered array whould be faster than searching a string in an unorded table but when i switched to tables i've noticed a significant reduction in perf warning spamming in the console (i wish there was a better way to benchmark vscripts though). I'm open for discussion about this. My opinion is that if there is no other particular reason to switch to arrays, other than for better memory usage, i still think that a faster code is preferable (less console spamming and also because the vscript can be interrupted by the C code if they take too long).

[smilz0]

Yep. sq_deleteslot calls SQVM::DeleteSlot which calls Raise_IdxError if the key isn't found. https://github.com/albertodemichelis/squirrel/blob/c02bf2dfd599a0b99b814d486512a3ee934667f1/squirrel/sqapi.cpp https://github.com/albertodemichelis/squirrel/blob/c02bf2dfd599a0b99b814d486512a3ee934667f1/squirrel/sqvm.cpp Don't know if this was changed in a later version though.

[LeGurdah]

Very well.

If you demand it, once I get back from my vacay, I'll get the arrays switched back to tables, and get the checks for the existence of keys back in, wherever deemed appropriate. (At least I commented the latter out in advance for situations like this!)

I do agree in that the game needs an absolutely better way of benchmarking VScripts. Otherwise, we're only able to engage in guessing games and rough estimates.

[smilz0]

Yep, would be cool to have a better way to do it other than putting the code in a for loop and look for the "script took xxx ms" warning in the console. Maybe there is already some console command/cvar for it that i missed? That is definitely possible lol.

Don't worry about the changes, take all the time you need.

[LeGurdah]

Okay. I returned from my vacay! I got the arrays reverted, and the "is key in table" if conditions reinstated.

I've the changes committed, but I'll need to do some testing tomorrow (this was, 'er, a fairly thorough late-night work.) Once I catch any bugs that slip by, I'll get them fixed and push the rest of the changes in the next commit.

On the bright side, I found more functions to refine along the way.

[LeGurdah]

'Kay, now I got the changes pulled off.

All clear to test away, smilz0!

[4512369781]

I made some changes and fixes.

in Left4Bots.FindBotNearestEnemy i wish not effect by "manual_attack_radius" so i use fixed value before, but it cause error if bots use melee.

foreach (witch in Witches)
{
    // fix for https://github.com/smilz0/Left4Bots/issues/84
    if (witch.IsValid() && NetProps.GetPropFloat(witch, "m_rage") >= 1.0 && (witch.GetOrigin() - orig).Length() <= radius && Left4Utils.CanTraceTo(bot, witch, tracemask_others))
        return witch;
}

in _Left4Bots.Events.OnGameEvent_ammo_pile_weapon_cant_useammo change conditions, and fix bug when having upgraded ammo.

if ((cWeapon == "weapon_grenade_launcher" || cWeapon == "weapon_rifle_m60") && (IsPlayerABot(player) ? Left4Bots.Settings.t3_ammo_bots : Left4Bots.Settings.t3_ammo_human))
{
    local ammoType = NetProps.GetPropInt(pWeapon, "m_iPrimaryAmmoType");
    local maxAmmo = Left4Utils.GetMaxAmmo(ammoType);
    local upAmmo = (NetProps.GetPropInt(pWeapon, "m_upgradeBitVec") & (1 | 2)) ? NetProps.GetPropInt(pWeapon, "m_nUpgradedPrimaryAmmoLoaded") : 0; //INCENDIARY_AMMO = 1; EXPLOSIVE_AMMO = 2;
    NetProps.SetPropIntArray(player, "m_iAmmo", maxAmmo + (pWeapon.GetMaxClip1() - (pWeapon.Clip1() - upAmmo)), ammoType);

    if (!IsPlayerABot(player))
        EmitSoundOnClient("BaseCombatCharacter.AmmoPickup", player);

    Left4Bots.Logger.Info("Player: " + player.GetPlayerName() + " replenished ammo for T3 weapon " + cWeapon);
}

[LeGurdah]

Your changes have now been merged in.

[smilz0]

Hey, sorry for the delay. The changes are a lot and affect pretty much anything. There is no way i can test this alone so i decided to let other people help test it. Soon i will add the other fixes, i will update the output .dpk and leave it there for testing before making the release. If you guys find any bug let me know.