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

Implement special victory and loss conditions for heroes and towns #8905

Merged
merged 88 commits into from
Jul 9, 2024

Conversation

Districh-ru
Copy link
Collaborator

@Districh-ru Districh-ru commented Jul 1, 2024

close #8717

This PR adds ability to set in Editor the next special conditions:

  • Capture a specific town.
  • Defeat a specific hero.
  • Lose a specific town.
  • Lose a specific hero.

It adds functions to get a list of heroes or towns present on the map.
изображение

изображение

изображение

@Districh-ru Districh-ru added improvement New feature, request or improvement editor Map editor related stuff labels Jul 1, 2024
@Districh-ru Districh-ru added this to the 1.1.1 milestone Jul 1, 2024
@Districh-ru Districh-ru self-assigned this Jul 1, 2024
@Districh-ru Districh-ru changed the title Editor victory hero town conditions Implement special victory conditions: kill hero, capture town Jul 1, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Clang-Tidy found issue(s) with the introduced code (1/1)

src/fheroes2/editor/editor_map_specs_window.cpp Outdated Show resolved Hide resolved
@Branikolog
Copy link
Collaborator

Hi. Great work on this PR!
I've taken a look at this PR and here are some issues I found:
image
When I start the map with such victory condition (the castle owned by red player) I automatically loose. There's no sense allowing checkbox for already owned castle to create such map, that is unplayable. (It works the same as in the original game though.)
In my personal opinion, that castle already owned by one enemy can be the special wining condition for all other players. So once it is captured by another player - then he wins. Why not?
Edge of the world 10.zip

image
In a case I play a single game - loose condition works as expected.
If I start multiplayer for 2 players - nothing happens, when AI captures my castle.
I suggest specifying loosing condition (castles or heroes) individually for each human controlled player so mapmakers could create fair scenarios for all players.

Edge of the world 1.zip

There's also one question that bothers me for multiplayer games: if one player fails (regarding loosing condition) what happens to his rest heroes and castles?

@Branikolog
Copy link
Collaborator

Not sure it's related to the current PR, but I somehow cannot select other players:
image
image

Also there's one more issue:
image

image

Blue and green players have similar castle set ups, but I can select starting faction only for green player.

@ihhub
Copy link
Owner

ihhub commented Jul 8, 2024

Hi @Branikolog , described by you conditions work exactly as in the original Editor. I believe it is a place of improvement for it but definitely not bugs in my opinion.

@ihhub
Copy link
Owner

ihhub commented Jul 8, 2024

@Branikolog and @Districh-ru , I fixed invalid race setup logic. Please take a look.

In order to reflect the change you have to resave the map to update its conditions.

@Branikolog
Copy link
Collaborator

Hi, @ihhub

Hi @Branikolog , described by you conditions work exactly as in the original Editor. I believe it is a place of improvement for it but definitely not bugs in my opinion.

The original editor in this regard was kind of broken in my opinion. I think we shouldn't allow to create such maps, that are broken and unplayable (in terms of conditions).
I hope you can fix the most apparent issues within future PRs.

@ihhub ihhub added the high priority Very critical change needed immediately label Jul 8, 2024
@Districh-ru
Copy link
Collaborator Author

Hi, @Branikolog, I've addressed the first issue: now the "Allow this condition also for AI" checkbox is available only for the neutral towns/castles.
изображение

Let's address the other issues with the special victory/loss conditions in multiplayer in the other PR, because it's more related to the multiplayer logic that most likely needs polishing.

@Branikolog
Copy link
Collaborator

Hi, @Districh-ru !

Let's address the other issues with the special victory/loss conditions in multiplayer in the other PR, because it's more related to the multiplayer logic that most likely needs polishing.

Sure! I find current implementation not really easy to catch, when I start selecting Human-only/Comp-human/Comp-only players and trying to set special conditions.

@ihhub ihhub requested review from zenseii and removed request for zenseii July 9, 2024 03:46
Copy link
Collaborator

@zenseii zenseii left a comment

Choose a reason for hiding this comment

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

@Districh-ru. I left some more suggestions for better phrasing. Please have a look when it is convenient for you.

Something about "for victory" and "for defeat" sounded awkward to me, so I tried my best to rephrase them.

src/fheroes2/editor/editor_map_specs_window.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_map_specs_window.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_map_specs_window.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_map_specs_window.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_map_specs_window.cpp Outdated Show resolved Hide resolved
src/fheroes2/editor/editor_map_specs_window.cpp Outdated Show resolved Hide resolved
@ihhub ihhub requested a review from zenseii July 9, 2024 13:11
@ihhub
Copy link
Owner

ihhub commented Jul 9, 2024

Hi @zenseii , I've addressed your comments. Could you please check again?

@ihhub ihhub merged commit 3dd745a into ihhub:master Jul 9, 2024
20 checks passed
@ihhub
Copy link
Owner

ihhub commented Jul 9, 2024

@Districh-ru , many thanks for implementing such features!

@Districh-ru Districh-ru deleted the editor-victory-hero-town-conditions branch July 9, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor Map editor related stuff high priority Very critical change needed immediately improvement New feature, request or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement missing victory and losing conditions in the Editor
4 participants