Skip to content

Consider next unit on planning retreat - #10954

Open
tau3 wants to merge 12 commits into
ihhub:masterfrom
tau3:feature/10480/consider-next-unit
Open

tau3 wants to merge 12 commits into
ihhub:masterfrom
tau3:feature/10480/consider-next-unit

Conversation

@tau3

@tau3 tau3 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Hello!

This is an attempt to implement #10480

If some other checks besides army and morale of the next unit are required, I'll add them

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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)

Comment thread src/fheroes2/battle/battle_arena.h Outdated
Comment thread src/fheroes2/ai/ai_battle.cpp Outdated
Comment thread src/fheroes2/ai/ai_battle.cpp Outdated
Comment thread src/fheroes2/ai/ai_battle.cpp Outdated
@ihhub
ihhub marked this pull request as draft August 22, 2026 09:13
@tau3

tau3 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

@ihhub, could you help me with CI, please? I can see only code style checking error, but there is no context: file, line, etc.

@ihhub

ihhub commented Aug 31, 2026

Copy link
Copy Markdown
Owner

@ihhub, could you help me with CI, please? I can see only code style checking error, but there is no context: file, line, etc.

Click on ... next to failed CI stage to View Details.

@tau3

tau3 commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor Author

@ihhub, could you help me with CI, please? I can see only code style checking error, but there is no context: file, line, etc.

Click on ... next to failed CI stage to View Details.

Ah, thank you! I had some of JS disabled, so that page rendered as almost empty for me (so code snippets), now it's fine

@tau3
tau3 marked this pull request as ready for review September 14, 2026 14:40
@zenseii zenseii added this to the 1.1.18 milestone Sep 18, 2026
@zenseii zenseii added improvement New feature, request or improvement ai Artificial intelligence behaviour labels Sep 18, 2026
@ihhub
ihhub self-requested a review October 10, 2026 04:18

@ihhub ihhub left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Hi @tau3 , I added several comments here. Would you mind to check them?

Comment thread src/fheroes2/ai/ai_battle.cpp Outdated
Comment thread src/fheroes2/ai/ai_battle.cpp Outdated
Comment on lines +751 to +766
size_t i = 0;
const size_t count = units->size();
for ( ; i < count; i++ ) {
const Battle::Unit * unit = units->at( i );
if ( *unit == currentUnit ) {
break;
}
}
i++;
if ( i == count ) {
i = 0;
}

const Battle::Unit * next_unit = units->at( i );
assert( next_unit != nullptr );
return { next_unit };

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Does this code check units for the next turn?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As I could understand, _orderOfUnits is like a circular buffer, hence I tried to find current unit in it and get the next unit by index.

const Army * currentArmy = currentUnit.GetArmy();
const Army * nextArmy = nextUnit->GetArmy();

const bool isSameArmy = ( currentArmy != nullptr ) && ( currentArmy == nextArmy );

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why do we need to compare armies instead of units?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought of that as the simplest way to check if two units belong to the same army. Otherwise I can make a function like bool Unit#isSameArmyAs(const Unit& unit) const to make it look more OOP (it would do the same logic inside though).

Comment on lines +155 to +158
std::shared_ptr<const Units> getOrderOfUnits() const
{
return _orderOfUnits;
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We should revert this change: see my comments above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have no objections here, but how to receive order of units otherwise? It's required to find the next unit.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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)

Comment thread src/fheroes2/ai/ai_battle.h Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

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)

Comment thread src/fheroes2/ai/ai_battle.h

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai Artificial intelligence behaviour improvement New feature, request or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants