In D2507#123985, @badosu wrote:

This would be ideal, unfortunately I don't have the skills to be able to figure out if this is a meta changing patch with a few games.

(Better not commit this too late in the dev-cycle then.)

Changed some JSDOC.

In D2161#131391, @Angen wrote:

Spreading this issue to melee units is not happy solution. Also I am not sure if this is actually needed for melee. Or target is 2 meters from me and I can reach in any direction, does not matter if target stays a bit higher than me or below or I cant reach it because my weapon is too short.

Notice melee units don't use parabolic range, so the only described case that would be true is that the range query would return entities which are actually out of range.

In D2993#131489, @Nescio wrote:

it's consistent to remove them.

True that.

The main reason for this differentiation above just Melee and Ranged is secondary attacks (which _will_ come and thus should be counted with). Then one can have a longer ranged melee attack, e.g. using a sarissa and one for close combat, e.g. a sword and Melee wouldn't suffice anymore.
We may just use AttackName and use the weapon there when possible (i.e. the above mentioned case) and revert to Melee and Ranged when it is either not possible (e.g. animals) or undesirable (e.g. structures).

Works as advertised.

Right, hence not having null be distinguished from null.

I do not see ai getting any valueable information knowing that some was defined as null instead of undefined.
metal: null equals to not including that line at all from simulation point of view so should be the same as `undefined'.

Fix research.
O (2*n) for filter so O (n).

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

@elexis provided the hint why the line is changed: if (template.wallSet) will always be false and thus it is not necessary to go through that?

Wonder why rP20046 changed this like this

Good question.

Rebased.

Thanks for the review @Angen :)

thank you

Conceptually seems OK and no issues on my end.

@Stan @vladislavbelov I'm very happy that MSAA can be implemented, which has further improved the image quality of the game. As for this patch, when will it be officially released?

@Stan New progress has been made on this issue. What is your next step in developing this feature?

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

Correctize

The correct verb is to correct.

You are right that elaborate trading balance doesn't belong to this patch but could be aimed for in another. I like the garrisoning idea because in reduces ship crowding and makes individual ships more meaningful/interesting as targets / for protection.

Personally I like the idea of traders boarding merchant ships too, however, unless <GarrisonGainMultiplier> is greater than 1 (currently it's 0.2), constructing more merchantmen is always more profitable than loading them with traders.

That being said, I wonder if they should be slightly nerfed in consequence, or not?

Maybe lower <DefaultArrowCount> or <MaxArrowCount>?

The primary concern I have is the names are essentially arbitrary: functionally, it's irrelevant whether something is labelled axe, spear, or sword. The names are based on visuals (the props used by the actors' animations). Moreover, one can't really expect template editors to view the attack animations of each individual entity, nor artists to check and correct the relevant simulation templates whenever they make art changes.

While reviewing noticed that an upgrade without a cost will add a weird newline in the tooltip. However this is a separate bug, patch incomming.

I think the approach of having a template-configurable i18n is good, I'm wondering if it shouldn't be an attribute of the attack:

<Attack>
    <Ranged name="Boiling Oil">
    </Ranged
</Attack>

This would avoid having to think of a name for the tag.

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

Weapon => WeaponName

Or make it useful by using sim-information in the tooltip? Although I guess PetraAI would break if one changes classes in-game.

Note too that the current implementation is broken: from the template we store it under visibleIdentityClasses. Hence the patch letting classes be modifyable, or the patch wanting classes in the simstate, should properly implement it.

This seems sensible to me.
The fact that Biremes were enabled at town phase dates from rP12240 which introduced tech requirements for warships. It seems to me that biremes have no particularly strong reason to not be in village_phase, given the commit.

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

Just updated the code again to move the ownership of the message in SendGameMessage to the member variable. Hopefully this addresses the outstanding issue.

• Move game message ownership correctly

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

• Update SendGameMessage to rvalue ref. Fix char types

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

refactor
@vladislavbelov thnx

The patch looks good to me, but I haven't tested it.

removing visibleclasses from guiinterface is reasonable
tooltips get data directly from template
petra does not call guiinterface
I did not check usefulness of other things in getentitystate

anyway

1.) Looks fine
2.) I think dropsite types could be displayed somewhere for already build structure, just like gather rates are

1.) At first look it is good
2.) In my opinion unit should try to store current resource if they differ as well, but that may be another diff
3.) If there is no dropsite, it would fail after moving to resource anyway, but it would be near point where player expects it to go when gives that order, I think it could be confusing why it is not moving, maybe we could inform player that unit failed to find dropsite for given resource (just idea)

Left diff, right svn. Looks good by me.

Tnx god, tnx angen.

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

Move the check to a loop below. This should help performance since most entities returned by the rangemanager will be in range. Also nukes a loop

Noted issue is gone now :)
(Although I cannot reproduce it even with the former patch.)

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

Replaces gl_FragData by gl_FragColor and removing additional draw buffer.

Properly internationalise tooltips. (The rest accepts review.)

In D2983#131611, @vladislavbelov wrote:

Have you compiled the game?

Yes :)

In D2983#131491, @Freagarach wrote:

Before:

After: