Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the Great Ogre to core #7325

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add the Great Ogre to core #7325

wants to merge 6 commits into from

Conversation

CelticMinstrel
Copy link
Member

This used to be a custom unit in Legend of Wesmere, but it was removed at some point.

This allows addons to safely use it without having to copy the files into their own addon directory.

This commit makes Ogre advance to Great Ogre. It may instead be desirable to make this an optional advancement.

@github-actions github-actions bot added Graphics Issues that involve the graphics engine or assets. Units Issues that involve unit definitions or their implementation in the engine. labels Jan 25, 2023
Copy link
Member

@knyghtmare knyghtmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing the attack/defense animation frames which were done by some UMC

great-ogre-attack-1
attack1
great-ogre-attack-2
attack2
great-ogre-attack-3
attack3
great-ogre-defend
defend
great-ogre-idle-1
idle1
great-ogre-idle-2
idle2
great-ogre-idle-3
idle3

# From War of Legends UMC Era
{DEFENSE_ANIM "units/monsters/great-ogre-defend.png" "units/monsters/great-ogre.png" {SOUND_LIST:OGRE_HIT} }

    [attack_anim]
        [filter_attack]
            name="cleaver"
        [/filter_attack]

        [frame]
            begin=-325
            end=-250
            image="units/monsters/great-ogre-attack-1.png"
        [/frame]
        [frame]
            begin=-250
            end=-150
            image="units/monsters/great-ogre-attack-2.png"
        [/frame]
        [frame]
            begin=-150
            end=-75
            image="units/monsters/great-ogre-attack-3.png"
        [/frame]
        [if]
            hits=yes
            [frame]
                begin=-75
                end=100
                image=units/monsters/great-ogre-attack-2.png
                sound=axe.ogg
            [/frame]
        [/if]
        [else]
            hits=no
            [frame]
                begin=-75
                end=100
                image=units/monsters/great-ogre-attack-2.png
                sound={SOUND_LIST:MISS}
            [/frame]
        [/else]
        [frame]
            begin=100
            end=200
            image="units/monsters/great-ogre-attack-1.png"
        [/frame]
    [/attack_anim]

@CelticMinstrel
Copy link
Member Author

Could you label which frame is which in your post? I think I understand but I want to be certain.

@knyghtmare
Copy link
Member

Edited comment with the frame names

@spixi
Copy link
Contributor

spixi commented Jan 25, 2023

Ogre is in Loyalists AOH and Young Ogre is also in Hornshark Island (for all factions with the exception of Undead, which get a Ghoul instead). This commit would affect MP. Would it be better to make this an optional advancement like Death Knight and Armageddon Drake?

@CelticMinstrel
Copy link
Member Author

CelticMinstrel commented Jan 25, 2023

Okay, so just to clarify… none of the three idle frames are the same as the base frame, right?

Do you have WML for the idle animation?

@CelticMinstrel
Copy link
Member Author

Would it be better to make this an optional advancement like Death Knight and Armageddon Drake?

That's an open question, as I mentioned in the first post. I don't know. It's not that hard to convert this to an optional advancement.

@spixi
Copy link
Contributor

spixi commented Jan 25, 2023

That's an open question, as I mentioned in the first post. I don't know. It's not that hard to convert this to an optional advancement.

The Ogre is a little faster and more bulky than the Swordsman (55 vs 68 HP), has other resistances (20% pierce, but 0% impact), but he has bad defense in Flatlands and does not benefit from TOD. The Swordsman can advance, but the Ogre not.

The Great Ogre is much more similar to its counterpart Royal Guard. Damage is literally the same (15×3 vs. 11×4), but it lost bulkiness in comparison to his counterpart (55 vs 68 HP for the Level 2 and 74 vs 76 HP for the Level 3). It takes more XP to advance an Ogre (80 vs base 60 XP). Maybe we should make GO make more bulky, but decrease his damage to make the units more different from each other. Suggestion: 14×3 (42 damage vs. 55 for the RG during daytime), 91 HP, which keeps the same ratio like the Level 2 units.

@knyghtmare
Copy link
Member

knyghtmare commented Jan 26, 2023

I don't know. It's not that hard to convert this to an optional advancement.

Keep the Great Ogre stats as it is and make it an optional advancement with a macro. Seems the best course of action.

Do you have WML for the idle animation?

I checked and I do not have it.

@CelticMinstrel
Copy link
Member Author

Do you have WML for the idle animation?

I checked and I do not have it.

Does the WML I committed for it look like it'll work?

data/core/units/ogres/Great_Ogre.cfg Outdated Show resolved Hide resolved
data/core/units/ogres/Great_Ogre.cfg Show resolved Hide resolved
die_sound={SOUND_LIST:OGRE_DIE}
[attack]
name=cleaver
#textdomain wesnoth-units
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the ephemeral textdomain change here is not required now since this is entirely in #textdomain wesnoth-units

Copy link
Member

@knyghtmare knyghtmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, my bad. yeah, still missing the optional advancement macro for this one

@spixi
Copy link
Contributor

spixi commented Jan 26, 2023

Please also update src/game_config_manager.cpp and data/tools/unit_tree/wiki_output.py to be in sync with data/core/macros/optional_unit_advancements.cfg

Edit: The fist one is not needed, because there was no deprecated advancefrom= key.
Edit2: ENABLE_WOSE_SHAMAN from #4226 and ENABLE_SAURIAN_SPEARTHROWER from #6912 are also missing in wiki_output.py.

@doofus-01
Copy link
Member

I guess the ogre portrait can serve for this one, but the animations are pretty minimalist. There is more work to be done, and by itself it wouldn't be a problem, but it has to be seen in the context of the current flood. I need to think about this before I can approve.

@spixi
Copy link
Contributor

spixi commented Feb 5, 2023

I thought about this a lot. Northerners got Orcish Nightblade in 1.17 by default and also as leader for AoH. Ogre is the only neutral unit of AoH Loyalists (all other units are lawful) and it would be weird when this is the only unit which cannot advance, so I think we could even try this without an extra define. What do you think? @knyghtmare

@knyghtmare
Copy link
Member

I thought about this a lot. Northerners got Orcish Nightblade in 1.17 by default and also as leader for AoH. Ogre is the only neutral unit of AoH Loyalists (all other units are lawful) and it would be weird when this is the only unit which cannot advance, so I think we could even try this without an extra define. What do you think? @knyghtmare

Ogre is in Loyalists AOH and Young Ogre is also in Hornshark Island (for all factions with the exception of Undead, which get a Ghoul instead). This commit would affect MP. Would it be better to make this an optional advancement like Death Knight and Armageddon Drake?

Weren't you the one suggesting it be an optional advancement in the first place? For MP-balance related changes, Hejnewar must be consulted and requires his approval.

@cooljeanius cooljeanius requested a review from Hejnewar February 6, 2023 05:57
@spixi
Copy link
Contributor

spixi commented Feb 6, 2023

Weren't you the one suggesting it be an optional advancement in the first place? For MP-balance related changes, Hejnewar must be consulted and requires his approval.
I was. But then I saw that other units in AoH also get changes. So I think, it will be fine. Speaking about Hornshark Island, the starting units (inkluding the Troll) are not that problem. Fire Guardian (which is in the fallback roster used for Dunefolk and all non-mainline factions) got an unconditional advancement to Fire Wraith in 1.14, which is totally fine. (AoH Dunefolk dominate this map with their cheap 23 gold Falconers or Striders anyway, so their Mudcrawler, Giant Rats and Fire Guardian are merely a joke.)

@cooljeanius
Copy link
Contributor

#7815 would add its own copy of the Great Ogre

@cooljeanius cooljeanius requested a review from nemaara July 28, 2023 17:15
Copy link
Contributor

@nemaara nemaara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm being honest, for me the sprite isn't core worthy. The unit is fine though.

Edit: just my opinion, doesn't have to be a blocker or anything if anyone else really wants this in mainline.

@cooljeanius
Copy link
Contributor

If I'm being honest, for me the sprite isn't core worthy. The unit is fine though.

It's the same as what's in EIR, isn't it?

@nemaara
Copy link
Contributor

nemaara commented Jul 28, 2023

It's the same as EIR, but to me I think it makes more sense for core to have a higher standard of art since it's more visible esp to UMC? At least that's the way I see it, but either way I'm not the one who has to approve the art side, so it's just my opinion.

@cooljeanius cooljeanius requested a review from Dalas121 October 7, 2023 05:33
@Dalas121
Copy link
Contributor

Dalas121 commented Oct 7, 2023

In a vacuum, I don't think the Great Ogre is visually appealing enough to be a core unit (despite my usage of it in EI). But I don't see its art changing anytime soon, and I don't think it's "bad" per-say, so I'd be fine with it (and possibly the ancient ogre) as optional advancements. No strong opinion either way.

Copy link
Member

@doofus-01 doofus-01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm against moving this into core for quality reasons. If there was some sign that someone was working on it, I'd have a different stance, but it's a bit late now.

Maybe this would be a good goal for BfW 1.19/1.20?

@Pentarctagon
Copy link
Member

Is this PR still being pursued?

@cooljeanius
Copy link
Contributor

Is this PR still being pursued?

As with #7326 I technically don't really need myself, as I've already copied the Great Ogre manually into my add-on that needs it (ALA), but it would still be useful for new add-ons, or for other authors, so I think it's still worth pursuing for those cases. @CelticMinstrel ok to assign you?

@CelticMinstrel
Copy link
Member Author

I still want to merge this, yes. It looks like some art improvements are needed first though, and that's probably outside my skillset, so I'm not sure I should be the one assigned.

@cooljeanius
Copy link
Contributor

I still want to merge this, yes. It looks like some art improvements are needed first though, and that's probably outside my skillset, so I'm not sure I should be the one assigned.

@doofus-01 maybe, then?

@CelticMinstrel
Copy link
Member Author

Possibly, though he didn't seem interested as of his latest post in here.

@doofus-01
Copy link
Member

It can stay open for a 1.19/1.20 project. Possibly all these "Move this unit to core" PRs could be better as an issue, if having old PRs lingering is bad.

@cooljeanius
Copy link
Contributor

It can stay open for a 1.19/1.20 project. Possibly all these "Move this unit to core" PRs could be better as an issue, if having old PRs lingering is bad.

I personally think it's fine to have old PRs lingering if they're still wanted, but then again that's just me, so... @doofus-01 ok to assign you for the art improvements, or was someone else supposed to do them?

This used to be a custom unit in Legend of Wesmere, but it was removed at some point.

This commit makes Ogre advance to Great Ogre. It may instead be desirable to make this an optional advancement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Graphics Issues that involve the graphics engine or assets. Units Issues that involve unit definitions or their implementation in the engine.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants