0ad-project/binaries/... That's not going to apply cleanly when someone tries to test your patch. Please either use arc (described in https://trac.wildfiregames.com/wiki/Phabricator#UsingArcanist), or at the very least ensure that you create the patch using the directory that has binaries/ and source/ as the root.

In D742#29187, @elexis wrote:

GameView.cpp:869

In D717#29070, @elexis wrote:

keyword option

(IMO it makes things too crowded, but it just comes down to personal taste.)

Fix oversight; the traders should only move if !possibleRoute, as well as !ship.

In D642#28784, @leper wrote:

In D642#27284, @Sandarac wrote:

(Also there is an unused CTerritoryManager member var (another non-existent class) in CWorld with a corresponding getter, a relic of the old simulation (before the big rewrite that replaced it with simulation2).)

Care to post another diff for that one?

Okay, will do.

The patch removes the early return and now matches with the trainableEntities() function.

I first had to do a bit more reading on how this function is used, but in fact I am still a bit confused about what this patch achieves (maybe I miss something obvious, or didn't read enough):

• In all 5 uses of researchableTechs(), the civ passed to it will always be the player civ.
• The trainableEntities() function, which is like a sister function, is sometimes passed a civ (which again is the player civ), or sometimes not, and so it is possible that it may return invalid templates.

Alright, the comment was addressed in the new diff, and I ran tests on naval maps with the new diff.

I believe the diff is good, although to play devil's advocate, I'm really not sure about adding support for arbitrary mods which may or may not be modified later at a whim, making the code irrelevant (there is, of course, no comparable quality control nor review process for Delenda Est, and this is typical for most game mods). And in any case, such added code which panders to mods will be irrelevant/useless in the vast majority of cases anyways, as most people do not use mods (so this could be seen as effectively adding dead code), but only add unneeded complexity which will need to be maintained. I know there is some support in f.e. the structure tree, but that is perhaps different.

I don't believe the AI will need adaptation to these changes. The AI never accesses the Builder component (through AIProxy or otherwise); it instead uses its own
buildableEntities() function in entity.js (which is basically the equivalent of cmpBuilder.GetEntitiesList()). And in any case, this change to GetEntitiesList() just adds a template existence check, and the AI already does checks for this in the appropriate places.

Use Vladislav's recommendation and add the function that deletes m_MapReader to the LoadRequests to be called when all map loading tasks are finished, instead of calling it from CGame.

In D638#25957, @elexis wrote:

Camera Bug:
The camera height only checks for the terrain and completely ignores the waterheight. Hence it becomes more likely to have the camera below the waterheight with rising water.
It's also relevant to fix for maps that don't change the waterheight later, because if the difference between the terrain and water level is great enough, then the camera will drop off unexpectedly when moving it over a river for example.

Thank you Vladislav for the comments!

SAFE_DELETE() m_MapReader once all map-loading tasks have been finished.

(Also there is an unused CTerritoryManager member var (another non-existent class) in CWorld with a corresponding getter, a relic of the old simulation (before the big rewrite that replaced it with simulation2).)

If you're not going to use arc, you might want to try creating patches from root directory of your working copy. Using your E drive as the root directory is not helpful.

In D672#26968, @wraitii wrote:

I like the idea, but there's really nothing specific here about overlays. I'd refactor this into an overload of CreateTexture (i.e. a no-argument version or something along those lines). You'd have to call it twice, but then it'd be slightly more generic.

If you were to do that, you might check most existing calls to CreateTexture because I'm quite sure that many other cases could be slightly abstracted away.

I don't really know this code; when adding the range visualization I just moved the code that was already in the old UpdateStaticOverlay into the new UpdateTexturedOverlay (I didn't change/look at the math).

In D601#24680, @mimo wrote:

Yes, that's an autostart and you need -mod=public

I meant I used the exact command ./pyrogenesis -mod=public -replay=commands.txt on the attached commands.txt, but got the errors I posted when doing so. I didn't pass the mod 'mod', but I did pass the mod 'public'. It seems like some sort of bug.

In D600#24684, @mimo wrote:

In D600#24657, @Sandarac wrote:

Thanks, this will be a great help for this issue, but still, as alluded to the trac ticket, I believe that HQ.isDangerousLocation() should be used as well when planing trade routes, even more so as this approach is less perfect than doing an actual computePath check.

isDangerousLocation() also checks the presence of an enemy army around, which is much shorter time range than a trade route.

Yes, and it would be preferable to include it for trade routes (eventually, Petra should check its trade routes periodically with this check imo, even if just using a straight line as is done here). But yes, isUnderEnemyFire is fine.

In D576#24598, @leper wrote:

I'm not sure if it is worth extra logic to handle the case where an actor changing technology is researched while the user is placing a building.

This does not seem to be handled, or is it?

Update SetWallPlacementPreview, remove uneeded QueryOwnerInterface call in GuiInterface.js (just use the player var already passed to the functions), remove old comment in cmpVisual, remove the return in SetActor.

In D601#24623, @mimo wrote:

commands.txt293 KBDownload

Replay which shows the problem on turn about 14000

Thanks, this will be a great help for this issue, but still, as alluded to the trac ticket, I believe that HQ.isDangerousLocation() should be used as well when planing trade routes, even more so as this approach is less perfect than doing an actual computePath check.

This commit seems to be the one that broke two hotkeys, see D584.

Okay, this fixes the garrison/ungarrison loop in the described case.

In D579#23883, @mimo wrote:

commands.txt7 KBDownload

a small commands.txt for testing the patch

The comments were addressed in the new diff, looks good.

I really like this texture for the new heal range, thanks for that (preferably just for entities with the heal component, not for healing auras).

rP19700

Remove typo fix that was committed, add early return if teamsLocked.

Fix typo in rP19194 in chathelper.js that throws errors.

Update.

The patch was tested, and it works as advertised.

Here is an example when enemy structures are close to the AI's territory, maybe there needs to be a check for structure, the units will get stuck in a loop indefinitely; there are three enemy houses below the civic center:

I think this trigger script is getting to the point that some of the logic/functions should be moved to a common place (TriggerHelper) so that other scripts can easily use them to handle spawning gaia units/make them patrol/attack, etc. (likely won't happen in this diff), as the lot of the functions are generic enough that they should be elsewhere.

Fix that.

Some tweaks.

Okay, I think it is good.

Include wall tower.

I tested this map, and I think it is nice from a gameplay perspective (didn't look at the code).

Update and allow space in the options menu.

This is good to see, it's good to have different textures for different range visualizations (although I'm a bit uncertain if this new texture should also be used for auras that happen to heal, it may be confusing to new players).

Fix whitespace.

Don't save a another var to the rangevis comp that doesn't need to be saved.

Seems good.

On Windows, this diff causes a crash deep in SpiderMonkey when opening the replay menu (not if there is nothing to load in the replay menu though).

Forgot TypeList.h.

In D545#22497, @mimo wrote:

We can always improve it later when somebody provide a way to reproduce such problems.

Sure.