Commit message can specify that imo. And anybody looking will find this diff and notice what happened.

Let's just refactor this function entirely instead. See inline comments.

I think returning paths in a deterministic sorted order always is best. IO is a known slow thing, so losing a few microseconds (even a few hunderds) on that seems completely irrelevant and _should_ be completely irrelevant.

I think the time taken sorting some files by C++ code is irrelevant.

Will ensure this is complete and commit

But usually I just use the selection drag box to select a bunch of units and then remove those from selection I don't want. This won't be possible after this patch. And triple clicking on a unit isn't easy if the unit is part of a big fighting crowd.
Also after this patch you have no possibility to see if any of your catas packed itself when it's part of a big fighting crowd.

Ok that's a bigger issue that wasn't obvious from just reading the diff today.

I think this should differentiate packed and unpacked siege engines. (Correct me if it already does)

Still like this as I think we should put all civ-specific techs in the civ folders.

This seems obviously good to me?
Unless I'm misunderstanding things this just affects the double-clicking behaviour?

We probably need to setup a team moment to test this easily, as it won't work with the lobby obviously.

I guess the lesson here is "everything you say can be used against you", as per usual :p

I don't think it's a good idea to put this file in zip. It would be better outside so user can easily update it. Apps usually don't even bundle the GeoLite2 DB and rather let users to download it themselves. Also it would be good to have path to the database configurable so user can download the new database once to some location and make all apps use it without need to taking special care of each app (well on unix like systems this could be also acomplished using symlinks). Though while there are already some apps using MaxMind DB version if you make 0ad require the CSV format I doubt any other app would use it too.

I feel like sorting always would be better than sorting sometimes, as it reduces opportunities for inconsistency. Given that I assume you've changed all instances of GetPathNames, this seems safe enough based on what they do.

Apparently the fork 0ad guys have a patch for this already, which makes me wonder why I'd be doing any further authorship on this diff.

Will the fact that it is 3 times slower impact performance.

After much performance testing, I've found that this variant using a vector and a deque is probably the most efficient as a map replacement, as it performs extremely well for iteration and only about 3x as slowly as our current EntityMap for other operations (which remains faster than unordered_map). The code is also smaller and neater.

Anyways, this is a positive change.

So maybe clarify 'starting inside'?

Do you suggest to assume that we don't have rays inside spheres, and if have then it's not the intersection, because we don't see them?

Well "discoverable", depends what you mean. Ultimately it's all in the code obviously.

Committed this as it fixes the issue, making svn playable again. There might be a better fix, but it sounds like it would require a more extensive unitAI rewrite.

(I guess it's possible to write a program that can detect if a function appears more than 100k times in a stack, but not sure that is nice to have.)

General remarks:

Yes not a very strong objection anyway, go ahead.

Not to be annoying but wouldn't it be easier to say "from -X to +X, and "Normal" has no bonuses"?

We shouldn't use such way of dynamic loading until we have atlases for textures.

You didn't qualify the "texture atlas", but if you're talking about just caching texture data, that should already be case with m_TextureCache of CTextureManager (other than it being orthogonal to this patch).

In D1745#70229, @vladislavbelov wrote:

In D1745#70225, @wraitii wrote:

While that is correct I think it would be reasonably easy to adapt and thus can probably be merged regardless - I'll defer to your opinion but it doesn't seem like we'll use this feature _intensively_.

The network dialog (and possibly the lobby list) should be able to display country flags in the OList (#3787).
But creating an XML file with 250+ times the same icon definition seems not only tedious to create but also tedious to maintain.

The 250 textures isn't the intensive usage? I'm not sure how old video drivers will handle that. It's true that we already have many small textures, but we significantly increase this number here.

If we're commiting this patch now, I want to see some warning about the problem above.

While that is correct I think it would be reasonably easy to adapt and thus can probably be merged regardless - I'll defer to your opinion but it doesn't seem like we'll use this feature _intensively_.

I'm guessing the "OOP-ness" is the fact you define interactions in the JS instead of inside the XML?

Yeah I don't think we can really do this without adding a lot of JS overhead to the FSM, sadly. Maybe newer SM has better debugging support for this kind of issue?

This would be a workable approach on OSX but not to actually build a bundle - that's the snag I've hit in the past and I'm trying to fix with D1483.
The problem is OSX always links dynamically if it can, which leads to bundles that either:

• Have a huge internal Frameworks folder - which increases download times and still requires custom code on our side to support
• Can't be started unless the target computer has also locally-installed compatible libraries (in the same places).

To link statically on OSX, it needs to see only static libraries. This is what happens when we manually compile the libraries.

Might as well make this revision useful. BOOM. Massive SFINAE for this new container.

I'll give a go at this in the afternoon I think. Hopefully I should be done with a first working version of my componentManager map>entity map rewrite thingy by then.

Bump years before committing but this compiles and is nice.

Modified after itms' comments.

This type of check was done already in AllocateEntityHandle to centralise in a common function.
IMO like that it's fine to commit, it's more "canonical" and less tricky.

The loading order is deterministic / alphabetic

So either the ENSURE must ignore local entities, or local entities must be flushed before the serialization attempt occurs.

In this situation given that it won't change the gameplay I would procure an MP replay, add Profiler2 profiling around affected functions, and replay once with vector and once with deque, then use the profiler2 interface to compare. There's a guide in trac on how to use Profiler2.

Sorry, I'm going to be a little snarky, but there is a test plan for this revision that says:

Add a (rather long) battery of tests on the hierarchical pathfinder specifically.

Ah right, sorry. But I'd be interested in knowing how much of the cases are actually ent == INVALID_ENTITY. It doesn't sound like there are a lot of cases where an entity would be deleted several times?

I really think all the things that happen after this early return are inexpensive. In particular, when posting messages, if the entity is actually fully destroyed, no component subscribed to the message will be found, so there won't be a lot of overhead. If it's not actually fully destroyed, the patch is wrong (I do not think that's the case, but I might be wrong).

I believe that's incorrect: components that are globally subscribed will received the message.

As a reminder of what we're trying to achieve here: the goal (to me) is not to provide something beautiful, or even something complete, but rather a necessary, minimal, and well-architectured stepping stone for other code. As the patch stands, it is usable for a very basic campaign, though it's missing plenty of things one would want, but I think that's fine, we can add those on top of this core stuff, so long as the general direction of the core stuff isn't stupid.

Answers to some comments.

What's weird looking with buildings is the 'falling' animation which jolts and doesn't fall vertically (i.e. along the Y axis), and this does look odd. The rubble might have pitch-roll, but current rubble handles this nicely enough.

The more I think about it the more I think the best thing to do here is to remove/amend the ENSURE. It dates from rP7276, and I'm not sure if it ever was useful. The Simulation's UpdateComponents cannot be halted and we won't serialize or deserialise in the middle of it, so any non-local entity destroyed is flushed at the end of the turn, i.e. the destruction queue only has local entities in it. That's fine because those don't get serialised anyways.

Do this in the flushing using the component cache, which also handles already-deleted entities.

The reason for the crash is that find() didn't expect to be called with an entity below FIRST_VALID_ENTITY_ID, i.e. INVALID_ENTITY. This should return end() instead so I need to amend this code.

Right idea, terrible implementation

This is actually WAD, as mirages are hidden when the original entity is visible, so we need to clean them up, and destroying INVALID_ENTITY is supposed to work.

This destroyed INVALID_ENTITY on some maps (which triggered the crash in rP22029).

Rebase, address the bulk of elexis' comments above. Github branch up-to-date https://github.com/wraitii/0ad/tree/D11_campaigns .

Reupload to run on SVN now that tests have been merged.

So actually aura icons can't be merged with the modifications manager because they're only shown when the source is selected, which I forgot.

I don't think any global aura had an icon. Only local ones.

Migrate auras to the ModificationsManager, which allows me to delete AuraManager entirely (which is entirely redundant).

Requested changes to bump the year, really?

s0600204 isn't retired.

Ah, right, mistakenly assumed given the date of the last revision and didn't check.

Well that's kinda what I needed to know. Indeed this should get committed by whoever is available ASAP then. Will do later unless someone else's done it first.

Clarify head comment and revert the test-dumbing-down from D1722. The changes are strictly cosmetic.

Make tests compatible with svn, comment out what will work with D8.

Bump year and commit