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

rP23681 :) So a file in data/template_helpers/damage_types.

Add back context

In D2737#116901, @Freagarach wrote:

(Damage type file for translation.)

Use Cstr instead of std::string and replace std::stringstream by fmt::format following advice from @wraitii

(Damage type file for translation.)

I believe that is because initially one could set all the animations in one variant, and be done with it. With the apparition of carrying animations, and later with approaching animations, things became more complex because such animations have to be defined in different groups in order to work.

A classic grep won't suffice, you'll have to do it semantically to check variants.

For that one for example:

romans/hero_cavalry_swordsman_marcellus_r.xml:0

You will have to check "biped/rider/cavalry/base_swordsman_ready_shield.xml" which appears ok.

That's quite confusing! Why is that?

Good to have that done.

If I do it, will you check for completeness?

Sure, checking for completeness is the easy part, that takes just a few seconds.

In rP23697#42402, @Nescio wrote:

I guess I missed some. Feel free to make a patch :)

You did this one, are more familiar with the art files, and have commit access, so it's probably better if you do it. :)

I guess I missed some. Feel free to make a patch :)

You did this one, are more familiar with the art files, and have commit access, so it's probably better if you do it. :)

I guess I missed some. Feel free to make a patch :)

And healers?

[units]$ grep -c attack_slaughter */*healer*.xml
athenians/healer.xml:0
carthaginians/healer.xml:0
celts/healer.xml:1
hellenes/healer.xml:0
iberians/healer.xml:0
kushites/healer.xml:0
mauryas/healer.xml:0
mauryas/hero_healer_chanakya.xml:1
persians/healer.xml:0
ptolemies/healer.xml:0
romans/healer.xml:0
spartans/healer.xml:0

Great, all infantry can now slaughter. How about cavalry?

[units]$ grep -c attack_slaughter */*cavalry*_r.xml
athenians/cavalry_javelinist_a_r.xml:1
athenians/cavalry_javelinist_b_r.xml:1
athenians/cavalry_javelinist_e_r.xml:1
athenians/cavalry_swordsman_a_r.xml:1
athenians/cavalry_swordsman_b_r.xml:1
athenians/cavalry_swordsman_e_r.xml:1
britons/cavalry_javelinist_a_r.xml:1
britons/cavalry_javelinist_b_r.xml:1
britons/cavalry_javelinist_e_r.xml:1
britons/cavalry_swordsman_a_r.xml:1
britons/cavalry_swordsman_b_r.xml:1
britons/cavalry_swordsman_e_r.xml:1
britons/hero_cavalry_javelinist_boudicca_r.xml:1
britons/hero_cavalry_swordsman_cunobelin_r.xml:0
carthaginians/cavalry_javelinist_a_r.xml:1
carthaginians/cavalry_javelinist_b_r.xml:1
carthaginians/cavalry_javelinist_e_r.xml:1
carthaginians/cavalry_spearman_a_r.xml:1
carthaginians/cavalry_spearman_b_r.xml:1
carthaginians/cavalry_spearman_c_r.xml:0
carthaginians/cavalry_spearman_e_r.xml:1
carthaginians/hero_cavalry_spearman_maharbal_r.xml:0
carthaginians/hero_cavalry_swordsman_hamilcar_r.xml:0
gauls/cavalry_javelinist_a_r.xml:1
gauls/cavalry_javelinist_b_r.xml:1
gauls/cavalry_javelinist_e_r.xml:1
gauls/cavalry_spearman_c_r.xml:0
gauls/cavalry_swordsman_a_r.xml:1
gauls/cavalry_swordsman_b_r.xml:1
gauls/cavalry_swordsman_e_r.xml:1
gauls/hero_cavalry_swordsman_vercingetorix_r.xml:0
iberians/cavalry_javelinist_a_r.xml:1
iberians/cavalry_javelinist_b_r.xml:1
iberians/cavalry_javelinist_c_r.xml:1
iberians/cavalry_javelinist_e_r.xml:1
iberians/cavalry_spearman_a_r.xml:1
iberians/cavalry_spearman_b_r.xml:1
iberians/cavalry_spearman_e_r.xml:1
iberians/cavalry_swordsman_a_r.xml:1
iberians/cavalry_swordsman_b_r.xml:1
iberians/cavalry_swordsman_e_r.xml:1
iberians/hero_cavalry_spearman_indibil_r.xml:0
kushites/cavalry_javelinist_a_r.xml:1
kushites/cavalry_javelinist_b_r.xml:1
kushites/cavalry_javelinist_e_r.xml:1
kushites/cavalry_spearman_a_r.xml:1
kushites/cavalry_spearman_b_r.xml:1
kushites/cavalry_spearman_c_r.xml:0
kushites/cavalry_spearman_e_r.xml:1
kushites/hero_cavalry_spearman_harsiotef_r.xml:0
kushites/hero_cavalry_spearman_nastasen_r.xml:0
macedonians/cavalry_javelinist_a_r.xml:1
macedonians/cavalry_javelinist_b_r.xml:1
macedonians/cavalry_javelinist_e_r.xml:1
macedonians/cavalry_spearman_a_r.xml:1
macedonians/cavalry_spearman_b_r.xml:1
macedonians/cavalry_spearman_c_r.xml:0
macedonians/cavalry_spearman_e_r.xml:1
macedonians/hero_cavalry_spearman_philip_r.xml:0
macedonians/hero_cavalry_spearman_pyrrhus_r.xml:0
macedonians/hero_cavalry_swordsman_alexander_r.xml:0
mauryas/cavalry_javelinist_a_r.xml:1
mauryas/cavalry_javelinist_b_r.xml:1
mauryas/cavalry_javelinist_e_r.xml:1
mauryas/cavalry_swordsman_a_r.xml:1
mauryas/cavalry_swordsman_b_r.xml:1
mauryas/cavalry_swordsman_e_r.xml:1
persians/cavalry_archer_a_r.xml:1
persians/cavalry_archer_b_r.xml:1
persians/cavalry_archer_c_r.xml:0
persians/cavalry_archer_e_r.xml:1
persians/cavalry_javelinist_a_r.xml:1
persians/cavalry_javelinist_b_r.xml:1
persians/cavalry_javelinist_e_r.xml:1
persians/cavalry_spearman_a_r.xml:1
persians/cavalry_spearman_b_r.xml:1
persians/cavalry_spearman_c_r.xml:0
persians/cavalry_spearman_e_r.xml:1
persians/cavalry_swordsman_a_r.xml:1
persians/cavalry_swordsman_b_r.xml:1
persians/cavalry_swordsman_e_r.xml:1
persians/hero_cavalry_spearman_cyrus_r.xml:0
ptolemies/cavalry_spearman_a_r.xml:1
ptolemies/cavalry_spearman_b_r.xml:1
ptolemies/cavalry_spearman_c_r.xml:0
ptolemies/cavalry_spearman_e_r.xml:1
ptolemies/hero_cavalry_swordsman_ptolemy_IV_r.xml:0
romans/cavalry_javelinist_a_r.xml:1
romans/cavalry_javelinist_b_r.xml:1
romans/cavalry_javelinist_e_r.xml:1
romans/cavalry_spearman_a_r.xml:1
romans/cavalry_spearman_b_r.xml:1
romans/cavalry_spearman_e_r.xml:1
romans/cavalry_swordsman_c_r.xml:0
romans/hero_cavalry_swordsman_marcellus_r.xml:0
romans/hero_cavalry_swordsman_maximus_r.xml:0
romans/hero_cavalry_swordsman_scipio_r.xml:0
seleucids/cavalry_spearman_c_r.xml:0
seleucids/hero_cavalry_spearman_antiochus_the_great_r.xml:0
spartans/cavalry_javelinist_a_r.xml:1
spartans/cavalry_javelinist_b_r.xml:1
spartans/cavalry_javelinist_e_r.xml:1
spartans/cavalry_spearman_a_r.xml:1
spartans/cavalry_spearman_b_r.xml:1
spartans/cavalry_spearman_e_r.xml:1

Looks like I forgot to send my comments...

In case someone wants to play-test this on multiplayer, here's a mod:

In D2667#116877, @nani wrote:

Fair enough. Then I pass the commander to you if you want to try the grid approax.

Fair enough. Then I pass the commander to you if you want to try the grid approax.

In D2667#116873, @nani wrote:

std::array bool doesn't do bitpacking and its size must be known at compile time ( < c++2020 )

Unimportant, we'd use std::array when we store NB_PLAYERS items, which will be known at compile time.
The underlying storage for Grid would be a vector (of u8 perhaps), if that wasn't clear.

std::array bool doesn't do bitpacking and its size must be known at compile time ( < c++2020 ), std::vector allocates in the heap. current diff has many more chances than a more extensive refactor + upgrade to have less bugs. Is practically guaranteed same performance as previous because it doesn't leave to chance the optimisations that the compiler should make (is basically a wrapper of some operations and data encapsulation). So I don't see an extra benefit changing current diff to using grid.

For sure one can do this, but is it valuable? one needs 100% attention from the player to do so (one can't shift click anymore), so moving other troops got impossible and such

Looks good, but I have one concern. Player does not have to click in small distance from unit. Player can click further away in direction to dance so distance to target position will not be short.

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

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

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

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

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

Ah great. Never head of bintray before!

In D2766#116849, @Stan wrote:

I mean that the proposed link has more chances to go down than GitHub :) https://github.com/boostorg/boost/releases but after a second look it seems they only provide the source there, not a preconfigured version so nvm I guess.

I mean that the proposed link has more chances to go down than GitHub :) https://github.com/boostorg/boost/releases but after a second look it seems they only provide the source there, not a preconfigured version so nvm I guess.

In D2766#116846, @Stan wrote:

Isn't it safer to use the github mirror?

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

Isn't it safer to use the github mirror?

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

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

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.

In D2362#116819, @wraitii wrote:

I kind of get how you want to do this. It sounds sane.
But I've never done something with our animations/variants etc. so I'll need to look into it first. I'll do that when I find the time.
If you think it's a quick no-brainer for you, feel free to commandeer. ;)

It's quite easy: you set the animation variant, and in the groups you have something like:

<group>
  <variant name="default0" frequency="0.5">
    <animation name='Idle' ... />
  </variant>
  <variant name="default1" frequency="0.5">
    <animation name='Idle' ... />
  </variant>
  <variant name="build_field" frequency="0">  <- this never gets picked, unless the animation variant is "build_field", in which case it always gets picked
    <animation name='Idle' ... />
  </variant>
</group>

Ah, that's how I imagined it :)

I kind of get how you want to do this. It sounds sane.
But I've never done something with our animations/variants etc. so I'll need to look into it first. I'll do that when I find the time.
If you think it's a quick no-brainer for you, feel free to commandeer. ;)

In D2362#116778, @wraitii wrote:

Anyways, don't bother with XML, just do like carrying animations: define a variant build_{generic building name} (instead of an animation) and set it when building. Then make sure to create a group with the build animations only, and let the default one(s) have some >0 frequency, and name one "build_farm" with frequency 0. Then your units will do what you want automagically.
(you'll need to use "Idle" for your animation name when Idle, but that's OK).

A look at this with a pair of fresh eyes and some better understanding of JS and SpiderMonkey (I think).
Recap: how our GUI works:

• We have C++ objects, inheriting 1+ GUI classes. Most are children of IGUIObjects, though some (Scrollbar, TextOwner) are "capabilities" that are added by composition (-ish, they're inherited but those classes keep a reference to the GUI object instead of calling this). That's since D2325, following itself D2136. Fundamentally, that was a change towards Composition, which is preferred here I think. Anyways, irrelevant here.
• We have JS objects. JS objects can't inherit multiple prototypes. Thus, we can only have on JSCLass per C++ type _if_ composition over inheritance is the rule. Even with the rework above, I don't think we really fit the mold.
• The JS objects have a custom JSClass, with a custom prototype and a getter/setter hook (which gets moved around in SM52, but that's fine).

Oh and one final thing: I was wondering if you should prefer typeof attackTarget == "number" or attackTarget instanceOf Vector3D or attackTarget instanceOf Object.
Based on a quick reading of the SM code, I would expect typeof attackTarget to compile better/be faster.
It would be nice to check, though.
The basic check is to run an AI-AI game or maybe Combat Demo Huge in non-visual replay and check if the runtime is appreciably different.
After that (and I do mean after, because with a JIT you can't expect specific profiling to perfectly reflect absolute behaviour), you could use simple custom trigger to run the checks 1000 times and see what happens.

This diff has improved a lot since its early days, and it looks quite clean now, good work !
Please read the inlines, I've wrote a whole book' of em and I think you'll find them instructive.

cool, I'm happy to help

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

I've build this with D2671 aplied beforehand on alpine linux, afterwards i was able to run the game (i had some js errors in game setup view, but likely not related, wasn't on the highest revision but could do that if needed)

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

Moved the comment, moved the if bsd conditionals to the variable asignment for readability

It seems you could have kept the enumeration-like behaviour with a combination of the (undocumented, but still present in SM[latest]), from the jsfriendapi.h:

JS::AutoIdVector vec(m->m_cx);
JSID_IS_STRING()
JSID_TO_STRING()
js::GetPropertyKeys(m->m_cx, obj, 0, &vec);

That hardcoding seems to be a bad precedent.

UnitAI.SetDefaultAnimationVariant is actually not hardcoding, since it uses animation variants, and units can fallback to a default animation. gather_ predates it but should be updated. The code specific to resource-gatherer should probably be in resource-gatherer, but meh.

In D2440#116774, @wraitii wrote:

Very good idea to work on that :) .
...
It's kinda meh to do it in templates though, I think, and it forces creating entirely new actors (that might be OK though, because as elexis said in the general case our cameras don't really experience that much perspective).

Very good idea to work on that :) .
This needs some work before it can be committed, but nothing too huge I think.

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

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

• make Vulcan happy (copyright year)

No comment on the JS side as I don't think it's relevant yet.

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

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

Isn't square and circle more precise than ellipse and rectangle (which are more generic) ? a square being a rectangle while the converse is not true?

Yes, which is why they are renamed: a square used by an unit with an oblong footprint is no longer a square, but is still a rectangle.

• correct source files

In D2503#116754, @Nescio wrote:

As reported on the forums binaries/data/mods/public/art/textures/selection/actor.png binaries/data/mods/public/art/textures/selection/actor_mask.png textures are both used and should not be deleted. They are referenced in source/ps/TemplateLoader.cpp and source/simulation2/tests/test_CmpTemplateManager.h
Ideally at some point it would an actual XML file

Wouldn't it be better to simply update the lines in those two files to 128x128/ellipse.png? Or if that's unacceptable, move the actor.png to the mod mod?

As reported on the forums binaries/data/mods/public/art/textures/selection/actor.png binaries/data/mods/public/art/textures/selection/actor_mask.png textures are both used and should not be deleted. They are referenced in source/ps/TemplateLoader.cpp and source/simulation2/tests/test_CmpTemplateManager.h
Ideally at some point it would an actual XML file

Wouldn't it be better to simply update the lines in those two files to 128x128/ellipse.png? Or if that's unacceptable, move the actor.png to the mod mod?

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.

In D2763#116752, @Angen wrote:

they reshape for no reason when they are walking and order stop is given

they reshape for no reason when they are walking and order stop is given

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

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

fix one unit would not cheer as it stays in FINDINGNEWTARGET when receives Cheer order.

As reported on the forums binaries/data/mods/public/art/textures/selection/actor.png binaries/data/mods/public/art/textures/selection/actor_mask.png textures are both used and should not be deleted. They are referenced in source/ps/TemplateLoader.cpp

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

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

With this change, Checkbox is missing but text label is still there.
So it should be or only grayed out so set to enabled = false, or if hidden, text label should be hidden as well and in that case, there should not be blank space visible.
Also Promote selected units should be disabled when player is observer in regards why Control all units is disabled in that case.