Observation:

Is this intended by the author of the committed patch that LOGERROR is printed 7 times but the stack only once?

I'll say yes.

Instead, we should simply LOGERROR.

I'm not sure if I would agree with that part. This is not an "instead" but an "additionally" (JS exceptions should always be reported when they occur as such, so they can be caught with try/catch or otherwise abandon the JS execution and report the full stack etc).

the real advantage is merging the JS and the XML together.

This may be true for XML that is 1 or 2 lines long and becomes false with increasing amounts of XML.
Consider the GUI modders who want to add a frame with a label, an image and two button, perhaps even a COList with columns. How is that an advantage not being able to construct that from an XML file?

Because it's no fun if your fixes don't need fixing'

• Topic and possible syntax discussed with nani in PM on January 10th and Feb 1st 2020

(16:29:35) nani: let myButton = Engine.CreateGUIObject("button",{"text": "Ready", "onPress": () => warn("clicked"), "style": "ModernButton"} )
(16:30:18) nani: parentObject.attach(mybutton,[zindex])

surround stones and metal on the map

It's a feature?

Implications applicable to the patch depending on indeterminate values:

From a gameplay pov, perhaps not a bad idea to remove the attack againts docks (because playing a naval map against Iberians can mean they will spam fireships to destroy all attempted docks, establish safe economy while keeping an attack vector to the enemy, which can be sufficient to snowball the game, depending on enemy-dock distance and wood distribution).

Because it's no fun if your fixes don't need fixing'

I don't think the 2015 gamesetup is the logical end of any GUI script.

2015 gameseutp is the logical end of "historically grown code", i.e. N times just adding one more little thing with never refactoring. The antithesis is to first create a clean structure and then insert into that structure before it can become messy. This has worked well in the simulation entity component system for instance.

The preferred class was added in rP18765; I guess since elephants play the role of siege engines.

Palisades are weak and practically never used.

I can't say that's true.
They are especially cheap and multiple layers of palisades can buy sufficient time to train the necessary units to counter the enemy.
They are also very quick to be built since rP17312.
After 30min of gameplay time one can turtle up so much because of the cheap cost (and wood cost only) that it makes the entire gameplay sort of boring. In a balanced 4v4 with capable players, if the enemy doesnt turtle equally, he wont be able to progress unless he has an undestroyable mass of siege and soldier units.
So palisades may be weak in comparison to other structures, but they are have a high reward ratio if used correctly (leaving no holes, reinforcing most relevant entry points, multiple layers). (After 30min with all the wood upgrades one can most often gather wood very quickly)
There are many players who will never build walls because they are habituated to that and find it lame to build them (since it can degenerate gameplay into boredom etc.). If players would always play to their greatest benefit, then players would build them if the game hasn't been won yet at the point where palisades become low hanging fruits.

you've gone overboard with the files tbh

This is functional everywhere I've tested.

This applies to most versions uploaded, it was Vladislav who had provided me input values where the UI would show undesired non-rounded values (0.24999999 etc).
I don't know if the most recent version worked for *all* possible user input.
There were multiple patch designs as well.
I didn't commit the patch because I was still unsatisfied with it.
I don't recollect what it was, not unlikely more than one aspect. For example one big thing is that value representation is impossible for all user input, and the other is the complexity of having both rounding and step_size instead of only one of the two going on, and doing half the code in JS and the other half in C++, I don't recollect what else right now.

In D1931#117866, @linkmauve wrote:

I haven’t tested this patch, but it’s overall a much needed clean-up, so +1 for it!
I remember wondering why configuration was intricated like that, when I was trying to bring up GLES.

Make it clear where the warning is coming from when we try to convert a non-object to CColor.

In D2771#117727, @Dunedan wrote:

And in case it wasn't clear from the code, the trigger script spawns wolves, the wolves receive Move orders making the wolves attack the players units - so basically zombie wolves. It was added to make the map less boring. The wolves are Domestic because currently only Domestic animals can follow move orders […]

For me with this change the (now non-domestic) wolves move towards and attack player units after spawning the same way as they did before. What am I missing here?

Thanks for the patch and review, I had forgotton about that regression of mine.

The comments without parentheses will be understood as helpful by most readers.

I meant what I said - when someone uploads a patch removing something odd, then one should attempt to find the reason why it's odd. Often there is a reason for odd things being done and removing the odd thing may remove the oddness but also remove something intentional that the remover might not have intended to remove.
From the summary I can't distinguish whether you were aware that the wolf receives move-orders from the trigger script, which was the reason UnitAI was changed originally and then later changed to Domestic unit.

There are two questions which were not asked in the summary so far which also are the same question - why are they domestic units if that is odd? If there is something that is odd then the revision history can give you insight on why a commit added the change. So the second question which is the same as the first question is what the revision history and comments posted on that commit said.

UnitAI.SetDefaultAnimationVariant is actually not hardcoding, since it uses animation variants

It is not hardcoding which resource carry variants are chosen but is hardcoding which components can set a different 'default' animation variant as opposed to the other functions calling SetAnimationVariant and hardcoding the string concatenation format, which is acceptable for resource type animation variants but doesn't look applicable for building animation variants.

Verified that exactly one parenthesis is added that doesn't change the logic of this line and that it resolves the gcc warning, did not check the logic of 23690.

From what you posted, the uploaded patch shows the filename of the failed JS randommap testfile on linux but not on windows, whereas failed simulation tests shows the filename of the JS test on both platforms when not using a debugger.

(So still thanks and no objections for this iteration of this patch, just some thoughts and observations and this batch of inline comments may be ignored, and the one with parentheses even more so than the others)

According to Stans test, on windows terminal, simulation tests show the filepath of the error, but not the JS exception thrown - unless using a debugger;
and with this patch it will not show the JS stack nor the JS filename of a failed rmgen test;
therefore it needs to show the filepath in a failed error message as with simulation tests
(if we want to support rmgen test development on windows without debugger).

If the idea of the patch is to increase the nr 200 to a greater number, then the idea of the patch is correct, but it's time hasn't come yet if selecting many units is so slow that it delays the game (turn length time just for the selection).

• Engine.GetPlayerID() can change during the course of the game using the developer overlay "change player" option.
• Other hypothetical enabled properties could change arbitrarily.

error(foo) prints a message, but thats not a JS exception, one can raise one using throw new Error("foo").
The serialization test error is unrelated and should be reported in a specific ticket, excluded from here.

When having more than 1000 FPS, the menu panel is not animated anymore (millisecond delta = 0), reported by OptimusShepard on
http://irclogs.wildfiregames.com/2020-05/2020-05-03-QuakeNet-%230ad-dev.log

I hereby review this patch based on the grounds that it is a defect I introduced in the feature of rP23455.

(Whenever seeing this patch I can't stop to ask myself if sqrt(x) for 2*r times or computing x * x for pi/4 * r² times is faster for r < 5, respectively 5 <= r < 25. As in it seeming to be inviting to compute circle / line intersections and only iterating the points on the disk, but I dont know how many points on the wasted effort scale that yields.)

Inline comments for two versions of the patch, I suppose not much changed.
I did not perform dynamic analysis.
To be clear, I do not request any change or commit, only try to offer anecdotes of experience, inspiration and technical backgrounds where noticeable.

Can't compile this test file on gcc 10 and clang 10, see #5756. (I assume it is this commit since it introduced the test and it wasn't changed much later, but I didn't compare the function signatures.)

Disclaimer ahead - that I dont click on request changes means I don't request changes. I am only commenting, if the argument is convincing it can be followed, and it can be disregarded if the argument is not convincing.

In D2727#115103, @Imarok wrote:

In D2727#115077, @elexis wrote:

WithReturn can be avoided from the name, the functions are distinguished by signature already and do the same except return value.

Why? They have the same signature and only distinguish in the return value. Some parts of the gui need ScriptEvent to have no return value. (e.g. RecurseObject)

Is there a compile error if RecurseObject gets a function with ignorable return value instead of a void?
Aside from that one can still name the void and function the same way if both signatures are necessary and the compiler will chose the signature it needs.

WithReturn can be avoided from the name, the functions are distinguished by signature already and do the same except return value.

In D2296#111529, @Angen wrote:

What could be done is to have %(statusName)s: %(tooltip)s, %(effects)s, %(interval)s, %(duration)s %(stackability)s defined in template.
But duration and interval can be changed through aura or technology, what makes 4 options defined in template and 4 ifs in code what will make it easier to read.
Downside is 4 strings for every template.

I do not see how moving translate("%(statusName)s: %(tooltip)s, %(effects)s, %(interval)s, %(duration)s") to global would help. One would need to develop some key generation. Possibly concatenating some strings based on template values. Plus could be translation is done only once, but key building could be more costly than current translate calls.

Now there are 5 if branching needed, what is one more compared to first option and the same with second one.
One if branch removal with 4 strings in templates what cost memory is not exactly good trade-off, unless we do not care about additional memory.
Second choice is more questionable if should or should not be used. One could compare time spend in the function with current and second approach and see which one is better.

(And thanks again for writing the patch and the review)

You're welcome; D2222 and D2722 are somewhat related, and still need reviewers; interested?

There are many patches that I would be interested in reviewing. If I tell you here why I'm not an active reviewer I may lose my ability to review, no matter how politely I would express it.

You can look at https://www.transifex.com/wildfire-games/0ad/ . About a dozen languages are above 95%, several of which are 100% complete. I wouldn't worry about the translators.

That's good.

These strings are extracted and uploaded to transifex, so people will have to modify the translations. (I don't know about the current status of translations, whether theyre currently overwhelmed or not.)

11 blacklisted, 14 whitelisted

I see, from init at least 9 template cache variables. Then its fine to use whitelist (and author or reviewer need to become certain that the code still works deterministically (for rejoining clients), easy to have some difference, too late or missing init for rejoiner etc.).

	this.sortingClasses = this.template.SortingClasses.split(/\s+/g);
	this.shiftRows = this.template.ShiftRows == "true";
	this.separationMultiplier = {
	this.sloppyness = +this.template.Sloppyness;
	this.widthDepthRatio = +this.template.WidthDepthRatio;
	this.minColumns = +(this.template.MinColumns || 0);
	this.maxColumns = +(this.template.MaxColumns || 0);
	this.maxRows = +(this.template.MaxRows || 0);
	this.centerGap = +(this.template.CenterGap || 0);

In D2710#114523, @Stan wrote:

I wonder if you should use let for g_* in those scope. Var can be used out of the scope it's defined in

In D2710#114387, @badosu wrote:

One important remark, I noticed the build does not fail if there was a javascript error (as it happens when there's an invalid cache access on Area). Is there a way I can assert there were no javascript errors?

Language clause missing since translation of ToS since rP15021:

If this Agreement is translated into any language other than US-English, and if there is a conflict between the US-English version and the translated version, then the US-English version shall prevail in all respects.

TODO: Maximum pause duration for rated games (indefinite pausing allows someone to not have a loss accounted for), and outlaw leaving rated games more explicitly

Language clause:

If this Agreement is translated into any language other than US-English, and if there is a conflict between the US-English version and the translated version, then the US-English version shall prevail in all respects.

In D2710#114374, @badosu wrote:

I performed some tests on gear normal for 8 players, times are in seconds and retrieved from the map logger:

New (current implementation): 2.57 2.43 2.55 2.40 2.53
New (avoid acessing iterator properties): 2.55 2.65 2.52 2.59 2.54
Old: 2.95 3.12 2.96 2.88 3.26

So the implementation on this diff cuts time by 25% on total map generation for this map and one interesting fact is that avoiding property access on each iteration (use local x instead of it.x) does not have any significant impact.

Keep in mind I didn't use fixed seeds or biomes, this is just a small validation check. Also I tried to test other maps (especially ones that generate many small disks) but I found out that gear is the only with relevant discrepancy amongst the ones I tried.

Just for the record this.radiusSquared will be slower to evaluate than radiusSquared (multiplied by bbox area), but thats probably negligible (same for x vs it.x, then again using it.x avoids the set call).

The test supports only running all tests at once, which is fast enough.

(Just for the record, the Vector2D should only be created once, not once per iterated tile, then one can loop over it.x and it.y with Vector2D it. Iterating over tiles of a disk is happening in multiple places, so one might end up creating a common function.)

In D2707#114153, @Angen wrote:

@elexis what do you mean by So patch seems okay except for the missing = [] and cache is considerable. where are [] missing?

rP8296 introduced DeathType == "remain" without creating a new entity
rP12486 introduced the new entity to fix #1600 " Handle 'remain' corpses as discrete entities "
irc discussion on 2012-08-14, 2012-08-18, 2012-08-19, 2012-08-20, perhaps also 2012-08-27, 2012-09-05 (search term Deiz and corpse) on ​http://irclogs.wildfiregames.com/

I didnt see write to this.animations other than the one here (only took a superficial look), then it could be removed from serialization by providing a custom Serialize function that returns an object with all this properties except this one (there are existing components doing that).
Removal from serialization has the benefit of smaller savegames, faster rejoin transmissions and is mentioned at #3834.
The disadvantage of replacing this.foo = +this.template.someNumber by evaluating the +foo every call can be that it's slower, and I guess the number is written into memory again and again somewhere whereas in the other case its kept in memory.
If one provides a custom serialzation method one can keep the cache variables and not serialize them.
That again has the disadvantage of using a bit more memory.
So it's a tradeoff.
So I guess not serializing is the most important goal, and the secondary goal is to either optimize for runtime performance or memory use.
(One can estimate memory use by estimating the max amount of entities having that component, I guess its negligible or insignificant here, but I also wouldn't be surprised if it's a pattern that could add up. Then again JS components are supposed to be the ones that arent as performance critical as the C++ ones.)
(One could split the template rename from the serialization removal if one wanted to, but doesn't matter)
(So patch seems okay except for the missing = [] and cache is considerable.).
(Test plan should probably include dynamic testing, or be used to make the implied dynamic testing explicit)

Since rP23598 skips the globalscripts test if testing without public mod.

From http://irclogs.wildfiregames.com/2020-04/2020-04-19-QuakeNet-%230ad-dev.log

09:02 < Angen> Stan`: :D only difference with jenkins tests is how report looks like
09:03 < Stan`> Except here it would have tests were skipped :D
09:04 < Freagarach> It would be annoying to test and have that warning show up when making a total conversion, right?
09:05 < Stan`> Freagarach, Not really
09:05 < Angen> would not, tests are not skipped
09:06 < Stan`> "WARNING: Skipping component scripts tests (can't find binaries/data/mods/public/simulation/components/tests/setup.js)"
09:06 < Stan`> Is that not skipping ^
09:06 < Stan`> :D
09:07 < Angen> I meant in TS_Skip way :)
09:07 < Angen> they are just ignored

Right, the other way (sending Petra an offer to accept alliance if X wood and Y food are sent) is not implemented. And

No warning shown when another resource is tributed.

Then it ought to be correct and complete within the predefined scope.

Does it work too if one requested multiple resources? (Should it?)

(On style, x in y seems nicer than y[x] !== undefined, but if one has to test b in a && c in a[b] && d in c[a[b]] it can become easier to read if one writes it as a[b] !== undefined && c[a[b]] !== undefined && d[c[a[b]]] !== undefined since its in hierarchical order, so I always avoided the x in y statement. Also it has the advantage that the term evt.amounts[request.type] would appear twice, making it easier for the reader to notice, whereas it needs some more milliseconds of human evaluation to notice that the in-expression tests for the same as below. Not that it matters how you write it, especially in this diff.)

Just to be clear, you don't need to play with other humans, the important part is running in multiplayer mode in order to obtain a hashed replay for the purpose of testing that the patch works deterministically.
For example if the serialization of the obstructionmanager is good enough but something else is indetermnistic, it could break multiplayer, resumed replays or savegames etc.
It should be sufficient to start a cheat enabled multiplayer, spawn some hundred units, move units across the map, and then run replays with different numbers of threads, and perhaps test a late-observer join in a second instance of 0ad to test the serialization.
I guess you would have noticed already by many idle units if the replay was out of sync, but strictly speaking one can only compare performance if its known to reproduce the same replay. (The 1/numThreads performance benefit indicates that it works correctly, but one should be certain not to break determinism and serialization, see also https://trac.wildfiregames.com/wiki/SimulationRequirements#Determinism)
Thanks for working on this patch!

So, if the patch conserves the current game mechanics, and is actually deterministic, just adds threading, it sounds like a must-have in spite of the possible downsides.

But since there is no function in the determineAction to measure (unless calling the global is less performant that calling a local variable) one could perhaps measure only the initial state and take that as gain?

It would be an assumption that the first Object.keys().sort call has the same performance as the latter ones. (Unlikely, but in theory it could be optimized.)

It introduces another global variable

If a certain or an uncertain dev would finish the class rewrite, it would become a local variable, so it can be considered a global until then that is worth the perf reward.

Dunno if the "is ready!" substring should be bold too, I guess it's a bit arbitrary. One can be conservative unless one discovered explicit reason to change something (as was done in the class rewrite). I didn't compare all chat messages, you can decide, patch seems fine otherwise. I suppose I can risk an acceptance without having tested. Thank you again for writing the patch.
sdasdasdasd
asdasdasd
asds

Method:
I am a bit worried about the procedure of how the resulting patch is determined.

Previous revision D1662.

((Side node Documenting the use case / intention / objectives of the commit allows readers (including future self) to not have to guess that and to become able to search for use cases and find the commits implementing them.
Thats typically done in a summary of a revision proposal - if that is skipped it would be good to mention that briefly in the commit message to yield those benefits.)