[CPP] Exclude dynamic entities from query cache

[MowFord]

I affirm:

• [x] I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
• [x] I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
• [x] I have read and understood the Contributing Guide and the Code of Conduct.
• [x] I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

Currently, queryEntitiesByName caches the list of entities based on the search pattern. This is fine for static entities initialized at zone startup, but dynamic entities cannot fit this paradigm. I've racked my brain for various methods of doing this, and settled on what I imagine will not be the acceptable one. This technically works if your query starts with DE_, but the more I think about this the less I think we can cleanly have this function cache while also supporting dynamic entities. There's a few scenario that must be considered:

• Query for a particular pattern is run and has no dynamic entities in it. DE is inserted that matches the name pattern and query is run again. The memoization will return the previous list and not include the DE
• Query for a particular pattern is run and includes a DE. The DE is then deallocated and the query is run again. This time you get an entity object returned by queryEntitiesByName which doesn't actually map to an existing entity in the zone (since the cache uses std::move to store the entity list)
• The rest are variations on multiple DE with the same name being created/destroyed and attempting to query for the most current list.

Essentially, this PR only addresses the last one. Perhaps the first 2 scenarios are acceptable and we accept that queries don't get cached if they start with DE_ explicitly? I had an idea to check the list of entities in the cache for having nullptr records using zoneutil::GetEntity, but i'm pretty sure that's less efficient on average than just running the full query every time.

Finally, another idea I had was to set a bool to cache a query and if any result is a dynamic entity, then don't save the entity list at all. This would catch scenario 2 and 3, but still not 1. Again I'm somewhat convinced dynamic entities break the paradigm of this function's optimization altogether.

Steps to test these changes

This particular fix simply ignores the cache when zone:queryEntitiesByName(str) is run with str starting with DE_.

Inserting a dynamic entity in the zone and running the query multiple times can be shown to always return the real list of entities with the particular name.

[zach2good]

I'm not against an option to skip the cache during these lookups, but it raises more important points:

The DE is then deallocated and the query is run again.

This is not how DEs are intended to be used. The Dynamic in the name refers to the ID space that they live in (static: 0-1023, player: 1024-1791, dynamic: 1792+, or thereabouts) - not the lifetime of their underlying objects. I've been on the fence about removing the releaseIdOnDisappear flag because of how it's been abused and has allowed people to shoot themselves in the foot. If you're using DEs you should be allocating them up-front as soon as you can, and then reusing them exactly as if they were static mobs. This including storing their IDs and looking them up like how ID.mob.NAME is used, etc.

While it's useful and funny for summoning !fafnir whenever you feel like it, if you have custom content you know ahead of time that you want X mobs in Y zone at some point. So you can pre-allocate them and Spawn/Despawn them like you would with regular mobs.

I added this flag to try and assist with Horizon's custom era dynamis where they had every mob be a DE. They suspected they would run up against the hard client limits of how many DEs there can be in a given zone, when mixed with player pets, etc. The plan was for them to swap release the IDs of spawn waves once they were completed, but I don't know if they ever actually did it. Instead, releaseIdOnDisappear leaked into pretty much all of their custom content because it was easy way out, and has been probably in the top 3 causes for server instability.

Now that I've said my little speech, do we ever want to cache these lookup results for any entity in the dynamic ID space (1792+)? Player wyverns, summons, COP escort quasilumins, dynamic entities etc. The reason I suggest this for DEs is because you are the one that has created them at some point, and you have access to the new entity, and their ID - which you can and should be storing. queryEntitiesByName is useful for looking up static entities because you don't know the ID of a static entity until you've looked them up with queryEntitiesByName or GetMobByName etc.

It might make more sense to exclude entities from being added to the cache if they fall within that space.

[MowFord]

Very interesting. Always enjoy learning the history/intent on these systems. Honestly in most of the cases I've seen you are right that we don't actually need to release the id and not doing that would solve the problem we faced... But it definitely seems cleaner to release when unused.

More to the point though dynamic entities are very appealing for custom content and they seem well equipped (plumbing wise) to build and tear down as needed. I'm interested to know what stability issues arise from abusing their intent like that.

[zach2good]

The main problem they faced was caching of entities that later disappeared. Storing them in tables, or something has it targeted in some way, and then it disappears and the server will crash when that entity is used

[MowFord]

Back to your question i'm not sure we do want to cache DEs in this query, but I'm not sure there's a way to differentiate without looping over the whole npclist and moblist... which defeats the purpose of caching.

Insert DE and deallocate DE could edit these stored queries, but that might prove cumbersome and prone to weird errors

[zach2good]

Ultimately the cost of not using the cache is not that high on a modern computer. The lookup should be happening once or twice in the span of any content - not every tick or multiple times per tick, so I wouldn't worry about it.

[MowFord]

Just scanned the codebase and found most usage of queryEntitiesByName to be on init of things, some on despawn, a couple onGameHour and one within onMobFight:

[zach2good]

Yeah, that one in onMobFight is the opposite of what we want, especially if the caching for lookup is going to be disabled in certain circumstances. It's all-or-nothing in terms of how we treat it, otherwise it'll become confusing for people trying to use it.

[zach2good]

For the Jellys, that looks like it could easily be moved to the start of the BCNM and then never worried about again

[Frankie-hz]

The plan was for them to swap release the IDs of spawn waves once they were completed, but I don't know if they ever actually did it. Instead, releaseIdOnDisappear leaked into pretty much all of their custom content because it was easy way out, and has been probably in the top 3 causes for server instability.

Guilty as charged. We changed everything but Dynamis to not release anymore and will be changing everything to have pre-defined IDs. Will take some time to get caught back up.