@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:

Changed some comments.

Okay, forgot to mention it in the update message, but I've moved the immobility and garrisoned setting now to the "correct" locations. However, they are not correct here for turrets, since those change states, trigger leave and thus break the whole idea. (See D2379.)

Build failure - The Moirai have given mortals hearts that can endure.

Thanks for the review @bb!

@wraitii , @bb, @Angen ?

I can't review since I wrote part of the code myself.

Revert the move in gather.

I'm not particularly fond of the the forced integer wait time, but I failed to device a better way without major changes (i.e. using a range query and just setting a single timer to go to the next waypoint).

Good thing I added a conditional there..

In D2913#131562, @wraitii wrote:

Possibly final cleanup.

Did you try this in-game?

Possibly final cleanup.