Add functions for working with entity lumps

[nosoop]

Closes #1547.

This PR merges my Entity Lump Manager extension into SourceMod's core mostly as-is, with the primary change being that the lump is unlocked for writing during OnMapInit instead of during its own forward.

Making use of OnMapInit just about the only reason for merging into core — I'm open to alternative options if the team would like to push forward with its inclusion in some form.

Parsing logic tested against ~700 maps for Team Fortress 2, and a bug has been reported and fixed for CS:GO.

Also open to other changes like bringing the native names more in line with the existing ArrayList methods and such.

(PR is a WIP and not in working order — have to do some additional work for creating the handle type and to clean up the includes / license headers. Just putting the PR in now in case anyone has any input regarding its inclusion.)

[nosoop]

Have tested and confirmed that it's working now. This plugin is available as a sample for entity lump access; it's a copy of the extension's version with the replacement of OnMapEntitiesParsed with SourceMod's OnMapInit and the addition of a few extra diagnostic prints.

[asherkin]

This looks solid, I like the API a lot - thanks for doing the work here @nosoop! The usage does seem niche in total plugin count, but I think it's powerful enough (and stripper popular enough) that it does make sense to ship with SM for sure.

The implementation looks good, I haven't super dug into it yet but I know it's been well tested. The interaction between weakref and Handle could be a bit interesting, but I think it's definitely an easier way to go than dealing with handle cloning (and handles aren't the sanest in native land currently). @Headline I'd appreciate your eyes on the C++ stdlib usage here as an extra sanity check.

I think the only change to the implementation that I think it'd be good to attempt is moving the bulk of the code into the logic binary (LumpManager and the natives) - the per-engine builds really are slow and we're trying to share as much as possible, and it looks like we won't need much added to the bridge to accommodate this.

EDIT: Please could you include that test plugin in plugins/testsuite/ too - it looks like a really great reference.

[nosoop]

As long as there isn't any need for SDK-specific flags, moving it into the logic binary would be fine.

I'm not familiar with how the bridge itself would work. From skimming the source, it sounds like I should move the files into core/logic/, add either an interface or some functions to bridge/include/LogicProvider.h / core/logic/common_logic.cpp, then access them from core/sourcemod.cpp via logicore — is that right?

Aside, I'm also doing some minor refactors to move the SourceMod dependencies out of LumpManager.{cpp,h} so it can be compiled into a standalone binary like before.

Please could you include that test plugin in plugins/testsuite/ too

Will do; I'll push a commit with that.

Before I forget to mention it, there are currently no checks in place to ensure users don't insert double quotes [in keys / values]. I've considered it before, but I'm not sure which of the options would be preferable:

• Throw a native error if the string contains double quotes
• Implicitly convert double quotes to two single quotes
• Process the string as-is and have the server blow up in the case of malformed input (not ideal, but there weren't any safeguards in place in OnLevelInit)

[nosoop]

I've moved the entity lump code into the logic binary and confirmed that it builds and runs as expected on Windows / Linux TF2, and have also added the standalone entity lump parsing utility under tools/entlumpparser/; forgot to fix up the pathing so it reads from core/logic/ but it's otherwise buildable. Open to removing / fixing it up.

I'd also request to please add Co-authored-by: Mat <matan@mansheroff.net> to the squash / merge commit message if this PR gets accepted; they contributed the CS:GO-specific fix.

[Ilusion9]

Any news regarding this pull request?
Isn't a waste of memory to have lumps stored after the call of OnMapInit?
We cannot use them properly after that call, these lumps can change at any time (outputs can be added, keyvalues can be changed etc)

[nosoop]

Any news regarding this pull request?

Nothing on my end; just waiting on the merge or further feedback. I might retract the PR since it missed 1.11 to fill in for the now-removed OnLevelInit and there's no schedule for 1.12.

I can't think of a good migration path from the extension to the in-core version, which is why I didn't want to publish extension builds myself.

Isn't a waste of memory to have lumps stored after the call of OnMapInit?

The lumps remain available in case plugins have any reason to read the original data in the future. Should be able to pool the keys, though.

[nosoop]

Retracting this PR; extension builds will be made available within the next few days*.

I'm open to resubmitting this in a form that is backward-compatible with users of the extension in the future.

* unless a maintainer merges this into 1.11+ or gives me something actionable before builds go up, I guess — the important thing is that users on current stable builds are able to work with this

[nosoop]

Reopening this and moving the informal discussion with @Headline from Discord into the PR conversation here. Hopefully we can get this sorted out and into stable soon.

nosoop: [I assume 1.11] gets no feature updates.
asherkin: that's not true / the plan, and merging [this] PR into [1.11] is 100% the goal (and was a condition discussed when 1.11 was branched)

headline: What was the decision for weak_ptrs made from? Does it make sense for these references to expire?
headline: My guess is to handle erasures?

If I remember correctly, the idea was that EntityLumpEntry should only be retrieved on an as-needed basis and has no guarantee of existing once control is handed off to another plugin. If something else removes the entry from the global EntityLump, expiry is a way to indicate that attempted changes to the entry won't take effect on the map.

Still, a plugin shouldn't remove an entry it's working with, and it shouldn't expect anything to be preserved if it calls into another plugin, so I consider it developer error. I can expose a property to check for expiry, but in my mind, plugins should not have to worry about it; granted I'm sure people will find creative ways to break my assumptions.

nosoop: [...] I think the one nit I'd want checked at this point is handle permissions, can't remember if it was cloning or deleting that I was thinking of though

To clarify, the permissions currently in the PR (in sm_LumpManagerGet) use the plugin as the owner and core as the ident. I'm fairly sure cloning / deleting is currently allowed, but at some point I was considering removing the ability to clone + transfer ownership of the weakref.

Last thing I was considering was tweaking the method naming to match those of ArrayList (replacing Update with Set, Append with Push).

[nosoop]

Fixed up a bug introduced in the core-merged version that would cause issues if parsing failed partway into a file.

---

For those interested in testing, attached are builds of Windows and Linux versions compiled for supported SDK versions. The builds were compiled against entlump-1.11, which is this PR rebased against a somewhat recent 1.11 commit (5d9ddee causes rebase conflicts, presumably due to a file list being reverted to a previous ordering).

⚠️ The build is provided as-is and may not reflect the final state of the PR. If you are a server operator and don't want to worry about changes that may break plugins, wait until the PR lands.

package.zip (Windows) / package.tar.gz (Linux)

I've tested the build on TF2, so I assume there won't be any regressions for the few others that have run the extension; the lumpmanager component is practically unchanged.

If an entity lump parse failure occurs, it should report an error, but otherwise the server should continue to function as before.

[dysphie]

LGTM! Minor nitpick: Would it make more sense to turn EntityLump.Length() into a property? Especially since EntityLumpEntry.Length also exists

[nosoop]

Thanks for testing!

Would it make more sense to turn EntityLump.Length() into a property?

EntityLump is a methodmap with only static methods; properties can't be declared as static as far as I'm aware.
I could change the method name to GetLength to make it more obvious that it's a static function, though.