In D4155#182547, @bb wrote:

Even if unlikely not impossible. So we will be dependent on a few persons who have the code. Also this doesn't save all comments and discussions.

Er, we technically depend on a few persons now. If the servers were to crash irrecoverable and backups didn't work, we'd lose everything as well. Point is, I don't think this is much of an argument either way, though there are pros & cons for both.

Phab will keep running as long as we keep it running on our servers. Also I saw there were efforts to fork it.

AFAIK none support SVN, unfortunately. And well, it's true, but it'll also keep all its security flaws (if any) & its general slowness & never get any new feature.

In D4155#182536, @bb wrote:

Also what happens if github goes offline/kicks us (maybe not likely, neither impossible), we loose our code?

I think this is absurdly unlikely at this point, but even if GitHub deletes the repose unilaterally, git makes it so that we'd very likely have recent-is mirrors on several dev computers & on the lobby server itself. I don't think this should be a concern.

Why give away the sovereignty we have?

'Cause Phabricator is dying, see https://wildfiregames.com/forum/topic/42181-phabricator-is-no-longer-being-maintained-upstream/
Also sovereignty is essentially kept per the first point.

Why let the codebase fall apart, literally?

Other arguments:

• It's a good way to gain knowledge about GitHub usage, which will help should we decide someday to migrate somewhere else.
• the person who wants to work on this code wants to work on GitHub because it's easier for them

Say if I were to implement a new system of ratings, I did need to change the bots (as I need to compute the ratings) as well as some gui (i.e. I need to show the new rating). Having several repo's makes developing such a feature much harder, so is not wanted.

It's a quite realistic prospect to change the rating calculation without changing the rating display, and vice-versa.
The actual hard problem is deployment, and since we don't actually deploy the bots synchronously with SVN, we'd already have a lot of trouble if you wanted to implement a new system of rating.

In D4279#182399, @s0600204 wrote:

If we calculate the intersection between camera-view and ground for each corner:
("Vesuvius (6)" skirmish map, the camera orbiting the peaks in the centre of the map.)
Admittedly an extreme example, but the top edge wobbling all over the place happens on most maps.

This is getting off-topic, but maybe the right thing to do is make double-clicking select the control group instead of all similar units, if the entity is in a control group?

In D2175#182381, @marder wrote:

Would just be a bit sad if something that can be useful in many situations (selecting a bunch of units with one click)

I think you should merge this, it's a nice improvement & I'm sufficiently available to fix the CI if need be.

@Langbart: Check that it works with non Kushite units but if so yes.

I believe Vlad's point is that you could actually just query 4 height map points and draw a more accurate polygon. Though by itself the polygon would still be somewhat inaccurate because the field of view is not a perfect rectangle on hilly maps. The cost of doing that is likely not much worse than doing one query.

Yeah using formations for anything but walking / patrolling is various kinds of broken, which is why the default setting is not 'no override'.

In D3991#182204, @bb wrote:

Showing all the incompatible mods fills your screen with a lot of blur, a new user certainly shouldn't be seeing. So against this default change.

Well, a new user wouldn't have many mods, but point taken.

There is also a filter valid mods checkbox in the download screen. If anything it should be consistent.

Good point.

I really only see one usecase of the other default: if there are no compatible mods available. However in that case it would be much better to write that instead of the list of available mods.

How about I just adding "there are hidden, incompatible mods" as the last line in all cases where there are some

[disclaimer -> not tested]

I think TaskedResourceType might be a better fit than 'current'

In D4117#182026, @Ceres wrote:

@wraitii In the summary, you started with "Compile with this". While this might be obvious for you and other devs/experts, I am not sure whether I have to really compile (again). I did not compile again, as I have a working svn version of 0ad already.

You do have to compile again, after you applied the patch. Then it should work.

mh, this diff doesn't change UnitAI, so I'm not entirely sure why you had a problem in UnitAI?

In rP25789#53962, @Ceres wrote:

Could a map thumbnail be automatically generated in the game? Or is it possible to have thumbs automatically generated whenever a map/texture is updated in the repo?

This seems like a good idea 👍

I think you're doing redundant work on INVALID->Player when then entity spawns, which is bad as that's quite slow already.

In D3424#181342, @bb wrote:

Now I am not so sure if doing this on the cart houses is good gameplay wise (it renders the up pretty useless if one first loses pop to get a little more, s one won;t need to have more houses to start).

For what it's worth, this was intended to make upgrade less good, as it's currently very efficient since it takes no builder (something that e.g. Nescio didn't like in the Tower case IIRC). Giving a temporary debuf allows balancing the cost in resources & build time vs the ultimate bonus. Whether the current balance is good is quite debatable.

I kinda like the approach of a 3rd-party positioning class. I had some reserves about overcomplicating layout logic, but it doesn't seem bad here.

Looks to me rather unlikely that'd we'd get them to move quickly on this? Maybe they have an issue tracker where you can report that.

In D2623#181177, @bb wrote:

While I agree the duplication is bad, making a class from which to inherit comes with another problem: the this.chatFoo and this.chatFooCaption are different in each. Now one can unify this by storing the data under (the non-descriptive but general) this.chatObject and this.chatObjectCaption for all of them and and use that in the common class.
But this will mean in the common class, will be using data which is not initialized in that common class.

You can call super with the argument to the GUI object though? Maybe I'm missing something in your comment?

You're duplicating the code to fetch the caption size & align three time, which suggests that perhaps those 3 components should inherit from a common class (Chat Text Input?). GetTextWidth is definitely a better approach however :)

In rP25896#53803, @Stan wrote:

We actually need the translator's names to be able to update the credits.

If you've failed to find a system-wide setting, I think we just need to strip the email manually. To be honest, I'm wondering how relevant it is to bundle the translator names at all in the .po files, but the email is definitely a larger issue.

This is definitely a false positive, see https://code.wildfiregames.com/source/0ad/browse/ps/trunk/source/network/StunClient.cpp$337

yes

In D3995#180738, @wowgetoffyourcellphone wrote:

Well, obviously the player expects a map to play different when a Sahara biome is picked instead of an India or jungle biome, right?

Debatable. One could equally expect the setting to be purely graphical (and I would argue one is more likely to do that based on "RTS history", but that's also debatable).

In D4249#180595, @borg- wrote:

In my opinion yes, if you can build then you should be able to start constructions too.

It seems like Stan is only moving the demo maps. Which isn't super useful IMO, since those are already hidden by the 'demo' tag. Moving lower-quality maps is a different topic, which IIRC was discussed as 'community maps' but the naming really isn't that good.

In D4240#180178, @bb wrote:

In D4240#180128, @wraitii wrote:

However, [...] in the future.

In case things can directly be setted by the host, they need to be in persistmatchsettings and the like, so then we simply have the to/fromInit functions to set the values. As now the map determines all of it, those are not required, but adding them is easy as cake.

Your test plan is missing the MP case and I think this is where things are liable to get broken, and so I think a few changes would improve this diff.

My main question is 'how necessary is this' versus regular placers with high enough repetitions?
My understanding is that the most relevant piece is the code that picks the area surrounding a player base, but that's just a "neighborconstraint" (can't remember the proper name, think it's not that) large enough, no?

IMO, would be better on the bottom right side, overlapping a bit with the icon. I dislike the pure red, and would rather just use white.

In D4231#179750, @bb wrote:

In D4231#179742, @wraitii wrote:

(If you're interested in this bit, it would be _good_ to separate the persistent settings serialisation from the regular serialisation, too. Hadn't done so yet but it should be easy enough to implement.)

Did you ever notice this was the case prior to rP25077 and was broken in that commit?

In D4226#179685, @Angen wrote:

Well, they can annoy them in public chat as well so I don't see point there.

I believe we don't have a notion of Idle for buildings, since they don't use unitAI, hint hint @Freagarach ;).

This this is a good idea 👍

err, had misunderstood the problem, indeed code bug. Weirdly, did it correctly for seeds. Meh.
I also don't return there so I guess fine.

In D3995#178862, @marder wrote:

So the question really is: what should be decided by what?
Maybe a combination: biomes decide only if the maps doesn't specify it first?

@wraitii not sure if I understand your idea, would "balanced" and "realistic" be excluding choices or would there be options for both to choose between low and high? How would you handle decoratives ect?

Seems reasonable.

The lobby bot behaviour seems bit odd to me, but I don't see why this wouldn't work.
I believe I generally return true in pickRandomItems (or false, don't remember which means "I'm done").

I might be completely off-base here but I believe there _was_ a reason for not having MP messages in the lobby. That being said, I don't remember which exactly.

Be careful that this doesn't break behaviour, IIRC I had some issues with formations having low turn rate on A25.

Thanks for the patch :)
Quick glance looks good, I guess I'll formally review & merge this when I get back to some more activity. No ETA unfortunately.

Think I failed to notice that because that's always been broken on mac OS :/

In rP25135#53499, @Angen wrote:

So you removed attack from turrets, why is it bad to allow another units to garrison?
Why then should be able infantry to garrison when they (tower) cant shoot arrows anyway?
How is infantry more suitable to be able to hide from attacks in these towers as there is no other purpose currenlty for them to be garrisoned and not support units which has poor attack/defense capabilities copmared to infantry?

I've given some more thoughts to this and I'm not sure it's a good idea _by default_, at least for larger changes. It can break some maps, and it makes the maps less unique if the biomes determine more stuff.

Looks alright - haven't tested.

I think the imperative form looks fine. I'd like to see a comment added that FindWalkAndFightTargets must be called for all members or something, since that's not actually so trivial by reading I think (hence my original mistake).

Freagarach means using token modifiers to change the templates, instead of adding new templates. You can look at the tests I think for some examples.

In rP25843#53451, @vladislavbelov wrote:

The commit can be reverted after fixing the bug in the tex.cpp. Reported the original bug in #5022.

Seems reasonable.

Fixed in rP25843

Fix mistake noted by Vlad

And here it is in A23, showing that the fixed version is indeed correct wrt to the original, working implementation:

Oh and BTW here's the same spot in A24, confirming that the bug was indeed present there already, just hidden on Ngoro for whatever reason:

Note also that this effectively applied a MAX 1D convolution on the X-axis, so the heightmaps generated now are slightly finer/more accurate. Here's the change on Red Sea (Giant):
(NB: screenshots not taken from a replay so the rest of the map differs slightly)
Before: After:
(Note that on smaller map sizes, because of scaling, the difference is less noticeable).

In D4205#178639, @Stan wrote:

I have another weird bug, but it was there before your fix , when I remove the helmet prop, the sword changes color; It seems to sync with the helmet for some reason. Could be some weird variant sync going on.

That's normal, because it reloads the whole actor, and so since you removed a variant possibly the random number generator is doing things differently, thus different sword.

Adressing a few things:

[18:55:02] <elexis1> transforming to TEX_BGR resulting not in BGR is not a bug?

The 'spec', if there is any, is that grayscale doesn't convert to BGR/BGRA. Not a bug per se, just possibly surprising behaviour.

[18:56:24] <elexis> the texture library changed?

Nope'.

[18:56:37] <elexis> because we never had that bug in a23

It must have happened, but was either impossible to detect / not harmful as it only really shows up if something tries to read the height values 'off' the map (which the smoothing on ngorogoro does now).

[18:57:01] <elexis> so I assume it wasnt in a24 either
[18:57:14] <Freagarach> Correct.

Again, I don't think we've actually confirmed that - but if it's the tile class change, that's just a bug hiding another bug as usual.

[18:58:06] <elexis> and if converting to BGR doesnt result in converted to BGR then that might trigger bugs elsewhere too it sounds like
[19:09:30] <elexis> if that is a bug, then the rmgen code is correct and the diff would hide it

The rmgen code is incorrect by the sheer fact that it has a variable to handle different bytes-per-pixel, but doesn't actually do that. Either hardcode 4 and actually ensure it's BGRA, or fix the rmgen code. The latter is vastly more efficient for grayscale images, which are most height maps, so was my solution here.

[20:11:45] <elexis> the CODEC_CANNOT_HANDLE should be caught then and prevented

I don't necessarily disagree with that statement, but the texture loading code is dangerous stuff to touch.

Removing the 'debug' L90 breaks the code for grayscale images (which are all our height maps at the moment). The reason is subtle and annoying: the code attempts to convert to uncompressed BGRA, so presumably a bytesPP of 4. However, for grayscale images, this conversion fails silently with a CODEC_CANNOT_HANDLE when it tries to add the alpha channel (it would fail loudly when converting to BGR). Thus the image retains a bytesPP of 1. Thus the code is invalid: mapdata[offset] is OK, but the other two are reading other pixels, and in the case of the bottom-right pixels, they're actually reading random garbage.

In rP25210#53404, @Stan wrote:

This commit breaks the Run variation for some reason. (The actors don't have the same "seed" when in Run and Walk anymore after this)

Seems to work fine. On mac, we compile with --enable-unicode and I suspect most distros do too. OpenSUSE might well be an exception.

In D4179#177871, @Ceres wrote:

@wraitii
For my personal education: What is RB?