Successful build - Chance fights ever on the side of the prudent.

• Split "GetTargetPositon"-function in 2D and 3D.
• "filter(...)[0]" to "find()" for the time being.

Looking much better like that, I think.

Did you reproduce @Itms steps before the patch?

Successful build - Chance fights ever on the side of the prudent.

Looks like the conversation is coming back around to where we were at D1978, but with higher confidence and better understanding.

Successful build - Chance fights ever on the side of the prudent.

Do we really need that behaviour for wrong strings?

I don't think we do. Doubt we have any code in 0 A.D. that uses it, and I also doubt that any mod relies on that, because it's hardly useful anyhow.

Fixed check for timer.

Looks like formations are working.

• Added new functions to manage formation ranges.

I would certainly hope none otherwise MacOS has been behaving wrong for years.
The only place I can think of that parses strings to number is GUI sizes, which would previously parse things like "3b" as "0" instead of the 'correct' "3". I don't think there's mods or 0 A.D. code that relied on that - and I'm not sure we want that code to live.

Do we really need that behaviour for wrong strings?

(Re-upload of same diff with more context lines to aid in review.)

(following rP22378, rP22380)

Then: not deleting RecomputeBlurredNormalMap was an oversight, and it should be deleted.

In rP22297#34067, @elexis wrote:

This uses the skybox for reflections when reflections are disabled

Which is a contradiction.

It's not. Disabling reflections has never meant "show nothing". In fact, from the beginning reflections have been about entity reflection, so I guess your comment is correct. Again, I question your use of the concern option over a trac ticket.

Looks accurate to me :)

Either the function RecomputeBlurredNormalMap is considered obsolete as of rP15484, then rP15484 / rP22006 should have deleted not only some parts of it but all of it, or that code was useful and those two commits shouldn't have removed the calls.

In D1988#83470, @vladislavbelov wrote:

How many lines of our code depend on this behaviour (that wrong strings are still parsed)?

I would certainly hope none otherwise MacOS has been behaving wrong for years.
The only place I can think of that parses strings to number is GUI sizes, which would previously parse things like "3b" as "0" instead of the 'correct' "3". I don't think there's mods or 0 A.D. code that relied on that - and I'm not sure we want that code to live.

(1) So that the one reading this diff knows what's happening.
(2) So that cross-references are inserted

In D1985#83464, @elexis wrote:

(As mentioned byVladislav on IRC (http://irclogs.wildfiregames.com/2019-06/2019-06-21-QuakeNet-%230ad-dev.log) and in rP22297, this diff is correct because rP22297 reverted (and whitespace-formatted?) a hunk that was removed in rP22039.)

I don't understand what you want me to change.

How many lines of our code depend on this behaviour (that wrong strings are still parsed)?

In D1971#83463, @Stan wrote:

However I beleive that the spread parameter is made for that. One could use that to compute the circle.

I guess the spread is for unintentional differences in projectile landing location, where my idea is intentional spread.

Reverted by rP22032.

the other doesn't need to be called (apparently, since I removed coastal foam, which tbh I don't remember doing).

Coastal foam introduced in rP15576, seems to be present still, seems to be unrelated?
RecomputeBlurredNormalMap was introduced by rP15473, the only consumer of m_BlurredNormalMap was removed by rP15484.

(As mentioned byVladislav on IRC (http://irclogs.wildfiregames.com/2019-06/2019-06-21-QuakeNet-%230ad-dev.log) and in rP22297, this diff is correct because rP22297 reverted (and whitespace-formatted?) a hunk that was removed in rP22039.)

That sounds good and maybe actually better than being that precise though the shape should differ depending on the type of attacker. However I beleive that the spread parameter is made for that. One could use that to compute the circle.

I had a thought today: Usually, when someone wants to order ~10 catapults to attack ground a specific area, it is not desired that all catapults fire at the same location. I do not know whether it is possible, but it would be interesting to add an area to the attack ground order. So that if one clicks the button the cursor appears, but around the cursor a circle (perhaps filled circle) of the area to be bombarded. The circle would initially have a rather small radius (~5 m perhaps) but the radius can be increased/decreased by scrolling the middle mouse button (or pushing +/-). When the actual command is given, the catapults randomly choose a location within that radius to bombard, varying the location with every "PerformAttack".
How does that sound to you?

I tested, now the reflection works as after my commit.

In D78#14568, @mimo wrote:

while at it, here are a few defects i reported to you a long time ago on irc. Would be good to have a look at it some time

(apparently, since I removed coastal foam, which tbh I don't remember doing).

I also can't remember everything I committed immediately sometimes, but why mention this in the commit message, which should inform the reader of the relevant information, so it leaves a worse impression to the reader than it needs to. Just refs the commit id. A commit message shouldn't state all the things one doesn't know, because one should have found the answers to these questions prior to the commit.

Tested on a different setup again, seems to look better than the last test I did in D1402, so the change seems right.
The commit actually reverts a change introduced by rP15576.

refs rP21965

This uses the skybox for reflections when reflections are disabled

Successful build - Chance fights ever on the side of the prudent.

• Some testing on formations.
• Created "GetTargetPosition" in "UnitAI.js".
• Removed height-check in "Attack.js".
• Removed hard-coding of "Ranged"-type in "AttackGround"-order.
• ...

I couldn't convince myself that it would be right to support disabling capturing in the gamesetup for vanilla. But for mods it would be good to provide an easy way to implement that.

In D1971#83416, @wraitii wrote:

I think you should be able to make the regular attack work with formations too... Maybe a bit more work.

I'm trying, I'm trying ;)

I think you should be able to make the regular attack work with formations too... Maybe a bit more work.

As far as the opinion of a newbie with hardly any gameplay experience is valued, I agree with @elexis and @wraitii. Despite many edge cases, I do not think it is too hard te remove capture from all templates and stuff, but I haven't really searched for it yet ;)
A thought: When capturing GAIA animals (e.g. sheep, horses) gets incorporated, that should be treated differently as it may turn into a core aspect of the game.

In D1971#83392, @wraitii wrote:

Is the ATTACKGROUND state still necessary?

That state belongs to the Formation-part. I left it in so I can easily monitor what is happening when testing stuff (attackground vs non-attackground). And return to vanilla easily when things fail again.
The "Individual" "AttackGround" state is already merged with "combat".

as we are now compiling game with vs15, this is not needed

I mean to look at this after the Unit AI cleanup from the UM rewrite is mostly finished.

I kind of fail to see the point of replacing a constant with a function call. It's more code to read and provides us little.

I don't really like our current capturing, but I don't think we want a "no-capture" game mode in Vanilla 0 A.D., and as @elexis said mods can just remove Capturable.
If capturing is deemed bad enough that people would rather play without it, we should just rework the feature or remove it.

@wraitii Thoughts on this ?

I think it would be nice to have this.

@wraitii @Freagarach.

@vladislavbelov D1985

In rP22352#34056, @Stan wrote:

Raising concern for a warning reported by @Krinkle. No biggie but the target parameter isn't used anymore.
Why doesn't it set the the class attribute anymore ?

In rP22352#34056, @Stan wrote:

Raising concern for a warning reported by @Krinkle. No biggie but the target parameter isn't used anymore.
Why doesn't it set the the class attribute anymore ?

Well maybe you guys could try to use my conan patch ? You will need to change the copy to osx instead of win32 and likely edit premake libs.

Is the ATTACKGROUND state still necessary?

I'm abandoning this :) It probably can be reopened when we actually remove these references.

Raising concern for a warning reported by @Krinkle. No biggie but the target parameter isn't used anymore.
Why doesn't it set the the class attribute anymore ?

In D1483#83379, @Krinkle wrote:

I think this means versions are no longer controlled and libs no longer statically linked, thus the build no longer standalone and deterministic.
I'm still new, but I very much liked the idea of it being controlled in the repo, statically linked, and deterministic. I don't know how often this is an issue in CPP code bases, but for other code bases this typically increases bugs, things working for one dev but not another, harder to contribute due to rare issues specific to ones own environment, waste more triaging/patching/maintenance cost on lib stuff.

In D1691#83383, @Stan wrote:

Yeah I don't think that will work ever. I'm not sure where @fabio saw that it could be removed.

Weird that it compiled when I tried above. Guessing my compilation setup is a bit too customised for these kind of patches.

Yeah I don't think that will work ever. I'm not sure where @fabio saw that it could be removed.

I tried to test this on macOS 10.14 Mojave, but couldn't get it to work.

I think this means versions are no longer controlled and libs no longer statically linked, thus the build no longer standalone and deterministic.

Fixed the two remaining JSHint warnings as well.

In D1993#83352, @Stan wrote:

Maybe you can fix the last two warning (trailing dots)

Maybe you can fix the last two warning (trailing dots)

The auto fix failed I guess.

Successful build - Chance fights ever on the side of the prudent.

• Used eslint --fix to automatically fix the curly warnings as well per the 0AD coding conventions.
• Implemented the proposal for making the PETRA module files to use a slightly simpler structure. The previous structure had some extra indirection with variable overrides that didn't actually work (as ESLint correctly flagged), and turned out to be redundant anyhow.

Another error occurs when the AI has built a market:

In D1993#83328, @Krinkle wrote:

In D1993#83293, @Stan wrote:

Before this get committed you might as well want to fix the warnings that appeared in the Vulkan console.

The only remaining one should be for curly. Due to there being almost 50/50 adherence to that, I wasn't sure whether it is a style we still want to follow consistently or whether it might be something we want to disable and allow developer to choose on case-by-case basis whether curly braces are useful.

In D1957#83325, @Nescio wrote:

You're right, I mixed up, selling works fine. My problem is actually with the buying part. Instead of bartering a fixed 100 silver and getting +409 food in return, I want to buy 100 food and pay only the silver equivalent.
For comparison, if you go to the supermarket, you don't want to know how much bread you can get with €100, you want to know how much one bread will cost you.
Basically, there is a fundamental difference between a barter-based economy and a currency-based economy.

[EDIT] This patch doesn't really support currency, it supports restricted bartering, which works but is not exactly the same.

In D1993#83293, @Stan wrote:

Before this get committed you might as well want to fix the warnings that appeared in the Vulkan console.