Diff review notes

[ProditorMagnus]

Comments to remove

diff --git a/data/EoMa_data/abilities.cfg b/data/EoMa_data/abilities.cfg
index 94b0289d..6842852b 100644
--- a/data/EoMa_data/abilities.cfg
+++ b/data/EoMa_data/abilities.cfg
@@ -75,7 +75,7 @@
     [filter]
     [/filter]
     [filter_second]
-        x,y=$x2,$y2
+        #        x,y=$x2,$y2
         ability=AE_mag_killandrun{VALUE}
     [/filter_second]
     {VARIABLE_OP second_unit.moves add {VALUE}}
@@ -85,6 +85,9 @@
         text="+"+{VALUE}+" "+_"movepoints"
         find_vacant=no
     [/unstore_unit]
+    #[chat]
+    #    message=_"special test: kill and run"
+    #[/chat]
 [/event]

[ProditorMagnus]

    [dummy]
        #used by awaken
        id=AE_mag_swallow_filter
    [/dummy]

Consider prevent_awake instead of swallow_filter

[ProditorMagnus]

         id=AE_mag_collector{VALUE}
         name= _ "collector"+" +"+"{VALUE}"
-        description=_"This unit gains some HP added to its current health whenever it kills a non-magical unit. It also gains +1 max HP with each successful kill of a non-magical unit. This special works only on offense.
+        description=_"This unit gains some HP added to its current health whenever it kills a non-magical unit. It also gains +1 max HP with each successful kill of a non-magical unit.
+

Leftover commented code in ability

[ProditorMagnus]

    [dummy]
        id=AE_mag_bloodlust{VALUE}
        name= _ "bloodlust"+" +"+{VALUE}
        description=_"This unit gains hitpoints added to its current health whenever it kills a living unit. This ability works only on offense.
The amount of hitpoints gained:"+" +"+{VALUE}
        active_on=offense
    [/dummy] # wmlxgettext: [abilities]
[/abilities]
#code has been simplified and edited to work with aoe abilities like all-around
#[event]

comments

[ProditorMagnus]

         [filter_adjacent]
             ability=AE_mag_scavenger{VALUE}
         [/filter_adjacent]
+        [not]
+            [filter_wml]
+                [status]
+                    not_living="yes"
+                [/status]
+            [/filter_wml]
+        [/not]

filter for status directly

[ProditorMagnus]

+            [not]
+                [filter_wml]
+                    [status]
+                        petrified=yes
+                    [/status]
+                [/filter_wml]
+            [/not]

[ProditorMagnus]

ABILITY_AE_MAG_FORAGE is nice idea, I initially started writing about duration=scenario, but it is removed each turn.

[ProditorMagnus]

+    ## [attack_anim]
+    ## [filter_attack]
+    ## name=claws
+    ## [/filter_attack]
+    ## terrain_type=Q*,Mv
+    ## start_time=-450
+    ## [frame]
+    ## image="units/barbarians-rocs/{UNIT}-ns[1~5].png:50"
+    ## offset=0~0.25

[ProditorMagnus]

 [event]
-    name=unit_placed
+    name=unit placed,post advance
     id=AE_mag_chronoaura_start_event
-    [if]
-        [have_unit]
-            ability=AE_mag_chronoaura
-        [/have_unit]
-        [then]
-            {CHRONOAURA_MENU}
-        [/then]
-    [/if]
+    first_time_only=no
+    [filter]
+        ability=AE_mag_chronoaura
+    [/filter]
+    {CHRONOAURA_MENU}
 [/event]

unit placed should be enough, if using filter instead of if then first_time_only=no should not be needed

[ProditorMagnus]

+            #            {VARIABLE unit.variables.splitfire_active false}
+            [modify_unit]
+                [filter]
+                    id=$unit.id
+                [/filter]
+                [set_variable]
+                    name=splitfire_active
+                    value=false
+                [/set_variable]
+            [/modify_unit]
+            #            [unstore_unit]
+            #                variable=unit
+            #            [/unstore_unit]

comment

[ProditorMagnus]

+                    ## [animate_unit]
+                    ## flag=attack
+                    ## [filter]
+                    ## type={TYPE}
+                    ## side=$side_number
+                    ## [filter_adjacent]
+                    ## x,y=$x1,$y1
+                    ## [/filter_adjacent]
+                    ## [/filter]
+                    ## [primary_attack]
+                    ## name=key
+                    ## [/primary_attack]
+                    ## hits=yes
+                    ## [facing]
+                    ## x,y=$x1,$y1
+                    ## [/facing]
+                    ## [/animate_unit]

[ProditorMagnus]

-            [if]
-                [variable]
-                    name=actualgold
-                    greater_than_equal_to=4
-                [/variable]
+            {IF_VAR actualgold greater_than_equal_to 4 (

I would avoid IF_VAR for anything longer than few lines.

[ProditorMagnus]

-            {VARIABLE_OP unit.attack[{ARRAY}].damage add {ADD}}
+            [object]
+                silent=yes
+                duration=turn end#just in case something goes wrong with the damage numbers due to other factors, this ensures it will reset back to normal on next turn
+                [filter]
+                    id=$unit.id
+                [/filter]
+                [effect]
+                    apply_to=attack
+                    special_id=AE_mag_growingfury{ARRAY}{ADD}
+                    increase_damage={ADD}
+                [/effect]
+            [/object]

Looks like ARRAY parameter is not needed anymore

[ProditorMagnus]

-                time_of_day_id=dawn,dusk
+                #                time_of_day_id=dawn,dusk
+                time_of_day=neutral

[ProditorMagnus]

#for units whose leadership is weaker (or stronger) than their level. currently mostly designed for Recruitment Officer
#define ABILITY_AE_MAG_LEADERSHIP_FIXED LEVEL
    # Canned definition of the Leadership ability to be included in an
    # [abilities] clause.
    [leadership]
        id=leadership
        value="(25 * ({LEVEL} - other.level))"
        cumulative=no
        name= _ "leadership"+" {LEVEL}"
        female_name= _ "female^leadership"+" {LEVEL}"
        description= _ "Acts like leadership, but as if this unit's level was"+" {LEVEL} "+_"instead of the unit's actual level."
        special_note={INTERNAL:SPECIAL_NOTES_LEADERSHIP}
        affect_self=no
        [affect_adjacent]
            [filter]
                formula="{LEVEL} < other.level"
            [/filter]
        [/affect_adjacent]
    [/leadership]
#enddef

Looks like formula="{LEVEL} < other.level" should be >

[ProditorMagnus]

-#define ABILITY_AE_MAG_PAINABSORB_STRICT VALUE
-    [dummy]
-        id=AE_mag_painabsorb_aura_strict{VALUE}
-        name= _ "pain absorption (personal)"+" +"+{VALUE}
-        description=_"This unit gains some hitpoints added to its current health whenever an adjacent living unit is harmed with its attacks (directly or indirectly).
-The amount of hitpoints gained:"+" +"+{VALUE}
-    [/dummy] # wmlxgettext: [abilities]
-[/abilities]
-

Seems I need to check if EoMa unit test needs update

[ProditorMagnus]

-            lua_function=wesnoth.wml_conditionals.AE_not_rpg
+            lua_function=wesnoth.wml_conditionals.AE_mag_is_unbalanced

Lua file is not automatically imported, situations like this I need to manually catch

[ProditorMagnus]

    #clear coldauramemory from all sides
    {VARIABLE coldmemoryclear_counter 1}
    [while]
        {VARIABLE_CONDITIONAL frozenclear_counter less_than 100}#clearing 100 sides. a bit overkill but ensures scenarios with tons of sides still work properly
        [do]
            {CLEAR_VARIABLE coldauramemory$coldmemoryclear_counter}
            {VARIABLE_OP coldmemoryclear_counter add 1}
        [/do]
    [/while]
    {CLEAR_VARIABLE coldmemoryclear_counter}

Strange way but it should work. No real need to actually store sides to see how many there are.

[ProditorMagnus]

-    name=unit_placed
+    name=unit placed,post advance
     id=AE_mag_runeaura_start_event
-    [if]
-        [have_unit]
-            ability=AE_mag_runeaura
-        [/have_unit]
-        [then]
-            {RUNEAURA_MENU}
-        [/then]
-    [/if]
+    first_time_only=no
+    [filter]
+        ability=AE_mag_runeaura
+    [/filter]
+    {RUNEAURA_MENU}

[ProditorMagnus]

+                            {RANDOM 1..9999}
+                            {VARIABLE statue_id "statue"+$random}
+                            {VARIABLE statue_target.id $statue_id}

Probably random to reduce id conflicts, needs more careful review

[ProditorMagnus]

Not from this diff, but this is probably same case as all event based abilities which need dummy unit

    #AMLA didn't work, got scrapped
    #    [advancement]
    #        id=stun_amla
    #        description= _ "+Stun on magical powder; +2 max HP; +20% XP"

[ProditorMagnus]

diff --git a/data/EoMa_data/traits.cfg b/data/EoMa_data/traits.cfg
index 128281cc..c761c0f0 100644
--- a/data/EoMa_data/traits.cfg
+++ b/data/EoMa_data/traits.cfg
@@ -152,7 +152,7 @@
         [effect]
             apply_to=hitpoints
             increase_total=5
-            heal_full=yes
+            #            heal_full=yes

Doesnt look like any unit has this amla

[ProditorMagnus]

+#textdomain wesnoth-Era_of_Magic
+[multiplayer_side]
+    id=barbarians
+    image,name="factions/EoMa-faction-barbarians.png", "EoMa - " + _"Barbarians" # wmllint: ignore
+
+    type=AE_mag_Cyclops
+    {AE_MAG_BARBARIAN_RPG2_LEADERS}
+[/multiplayer_side]

I expect I need to ignore them - elif "mrpg" in fname and "mrpg2" not in fname:

[ProditorMagnus]

-[unit_type]
-    id=AE_mag_Chaos_Rider

probably just file rename, seems id is still valid

[ProditorMagnus]

    ## {EOMA_AMLA_DEFAULT_CONDITIONAL 2 veteran_then_fanatic_then_default (
    ## [and]
    ## trait=eoma_veteran
    ## [/and]
    ## [and]
    ## trait=eoma_fanatic
    ## [/and]
    ## )}

Break automatic conversion, creates invalid WML

    ## {AMLA_DEFAULT}
    {AE_MAG_AMLA_DEFAULT_CONDITIONAL 2 veteran_then_fanatic_then_default (
        ## [and]
        ## trait=AE_mag_veteran
        ## [/and]
        ## [and]
        ## trait=AE_mag_fanatic
        ## [/and]
        ## )}

[ProditorMagnus]

    ## {EOMA_AMLA_DEFAULT_CONDITIONAL 2 veteran_then_survivor_then_default (
    ## [and]
    ## trait=eoma_veteran
    ## [/and]
    ## [and]
    ## trait=eoma_survivor
    ## [/and]
    ## )}

[ForestDragon-wesnoth]

ABILITY_AE_MAG_FORAGE is nice idea, I initially started writing about duration=scenario, but it is removed each turn.

Glad you like the ability I made

Looks like ARRAY parameter is not needed anymore

Incorrect, it does still matter for units with multiple growing fury attacks.

[ForestDragon-wesnoth]

 [event]
-    name=unit_placed
+    name=unit placed,post advance
     id=AE_mag_chronoaura_start_event
-    [if]
-        [have_unit]
-            ability=AE_mag_chronoaura
-        [/have_unit]
-        [then]
-            {CHRONOAURA_MENU}
-        [/then]
-    [/if]
+    first_time_only=no
+    [filter]
+        ability=AE_mag_chronoaura
+    [/filter]
+    {CHRONOAURA_MENU}
 [/event]

unit placed should be enough, if using filter instead of if then first_time_only=no should not be needed

No, from my testing unit placed does NOT fire on advancement, so post advance is still important

[ForestDragon-wesnoth]

#for units whose leadership is weaker (or stronger) than their level. currently mostly designed for Recruitment Officer
#define ABILITY_AE_MAG_LEADERSHIP_FIXED LEVEL
    # Canned definition of the Leadership ability to be included in an
    # [abilities] clause.
    [leadership]
        id=leadership
        value="(25 * ({LEVEL} - other.level))"
        cumulative=no
        name= _ "leadership"+" {LEVEL}"
        female_name= _ "female^leadership"+" {LEVEL}"
        description= _ "Acts like leadership, but as if this unit's level was"+" {LEVEL} "+_"instead of the unit's actual level."
        special_note={INTERNAL:SPECIAL_NOTES_LEADERSHIP}
        affect_self=no
        [affect_adjacent]
            [filter]
                formula="{LEVEL} < other.level"
            [/filter]
        [/affect_adjacent]
    [/leadership]
#enddef

Looks like formula="{LEVEL} < other.level" should be >

Again, incorrect. In [affect_adjacent], other_unit is actually the unit with leadership. you can see it in mainline leadership too

[ForestDragon-wesnoth]

-[unit_type]
-    id=AE_mag_Chaos_Rider

probably just file rename, seems id is still valid

Yes, eoma has plenty of units that got renamed but keep their id for the sake of compatibility

[ProditorMagnus]

Looks like ARRAY parameter is not needed anymore

Incorrect, it does still matter for units with multiple growing fury attacks.

I guess object would add damage to both growing fury attacks, but also remove from both. Small UI difference in unit preview, can stay then.

[ProditorMagnus]

-#define ABILITY_AE_MAG_PAINABSORB_STRICT VALUE
-    [dummy]
-        id=AE_mag_painabsorb_aura_strict{VALUE}
-        name= _ "pain absorption (personal)"+" +"+{VALUE}
-        description=_"This unit gains some hitpoints added to its current health whenever an adjacent living unit is harmed with its attacks (directly or indirectly).
-The amount of hitpoints gained:"+" +"+{VALUE}
-    [/dummy] # wmlxgettext: [abilities]
-[/abilities]
-

Seems I need to check if EoMa unit test needs update

fail EoMa_Chainlady attacker xp if cfg.defender_dies and cfg.thirdparty_dies expected 48 but was 6 fail chainlady should have healed expected >0 but was 0 fail chainlady should have healed expected >0 but was 0 fail chainlady should have healed expected >0 but was 0 fail chainlady should have healed expected >0 but was 0

Looks like max xp is now too low that it levels up instead. And Chainlady no longer recovers hp, so validation not needed anymore.

    if attacker.type == "EoMa_Chainlady" then
        local chainlady_heal_amount = attacker.hitpoints -100
        if chainlady_heal_amount == 0 then
            check(chainlady_heal_amount, ">0", "chainlady should have healed")
        end
    end

[ProditorMagnus]

No, from my testing unit placed does NOT fire on advancement, so post advance is still important

I think that is bug then. https://github.com/wesnoth/wesnoth/issues/8267

[ProditorMagnus]

first_time_only=no

I think this part is not needed even with name=unit placed,post advance.

[ProditorMagnus]

https://github.com/inferno8/wesnoth-Era_of_Magic/blob/master/units/Sky_Kingdom/Master_of_Fire.cfg#L36 and https://github.com/inferno8/wesnoth-Era_of_Magic/blob/master/units/Sky_Kingdom/Master_of_Fire.cfg#L103 multiple defense tags in one unit

[ProditorMagnus]

ABILITY_AE_MAG_CIRCLE_OF_BANISHMENT2 "greater circle of banishment" event needs id.

+    [fire_event]
+        name=touched

if that is custom event might be safer to use prefix.