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.

Sign up for GitHub

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

Jump to bottom

[1.5.2] Pathfinder fixes #4038

Merged
merged 7 commits into from
May 29, 2024
Merged

[1.5.2] Pathfinder fixes #4038

merged 7 commits into from
May 29, 2024

Conversation

IvanSavenko
Copy link
Member

  • If original movement rules are on, it is not possible to attack guards from visitable object directly, only from free tile
  • Fixed bug leading that allowed picking up objects while flying on top of water
  • Hero can now land when flying from 'guarded' tile to accessible 'guarded' tile even without original movement rules
  • Interface will now use same arrow for U-turns in path as H3

@IvanSavenko IvanSavenko linked an issue May 23, 2024 that may be closed by this pull request
hero is unable to move #3993
Closed
nullkiller
nullkiller reviewed May 26, 2024
View reviewed changes
lib/pathfinder/CGPathNode.h Outdated
@@ -37,6 +37,7 @@ enum class EPathAccessibility : ui8
NOT_SET,
ACCESSIBLE, //tile can be entered and passed
VISITABLE, //tile can be entered as the last tile in path
GUARDED, //visitable, but in zone of control of nearby monster
Copy link
Member

Choose a reason for hiding this comment

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

GUARDED_VISITABLE?

Copy link
Member Author

Choose a reason for hiding this comment

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

Corrected comment.
GUARDED is used always when there is an active zone of control, whether there is a visitable object or not.

nullkiller
nullkiller reviewed May 26, 2024
View reviewed changes
lib/pathfinder/PathfindingRules.cpp Outdated
@@ -377,7 +379,7 @@ void LayerTransitionRule::process(

case EPathfindingLayer::SAIL:
//tile must be accessible -> exception: unblocked blockvis tiles -> clear but guarded by nearby monster coast
if((destination.node->accessible != EPathAccessibility::ACCESSIBLE && (destination.node->accessible != EPathAccessibility::BLOCKVIS || destination.tile->blocked))
if((destination.node->accessible != EPathAccessibility::ACCESSIBLE && destination.node->accessible != EPathAccessibility::GUARDED)
|| destination.tile->visitable) //TODO: passableness problem -> town says it's passable (thus accessible) but we obviously can't disembark onto town gate
Copy link
Member

Choose a reason for hiding this comment

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

Interesting comment here.

nullkiller
nullkiller reviewed May 26, 2024
View reviewed changes
lib/pathfinder/PathfindingRules.cpp Outdated

if(destination.node->accessible == EPathAccessibility::VISITABLE)
{
if (destination.node->accessible != EPathAccessibility::VISITABLE)
Copy link
Member

Choose a reason for hiding this comment

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

Feels like two exclusive conditions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot to remove this code, will fix.

nullkiller
nullkiller reviewed May 26, 2024
View reviewed changes
lib/pathfinder/PathfindingRules.cpp Outdated
@@ -377,7 +379,7 @@ void LayerTransitionRule::process(

case EPathfindingLayer::SAIL:
//tile must be accessible -> exception: unblocked blockvis tiles -> clear but guarded by nearby monster coast
if((destination.node->accessible != EPathAccessibility::ACCESSIBLE && (destination.node->accessible != EPathAccessibility::BLOCKVIS || destination.tile->blocked))
if((destination.node->accessible != EPathAccessibility::ACCESSIBLE && destination.node->accessible != EPathAccessibility::GUARDED)
Copy link
Member

Choose a reason for hiding this comment

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

In pathfinder it should be possible to express it with BLOCKVIS & destination.guarded or am I wrong?

Copy link
Member

Choose a reason for hiding this comment

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

Quite dangerous. Also I do not know how AI will react on it. And feels like we need to cover this with unit tests in order to refactor it any further

Copy link
Member Author

Choose a reason for hiding this comment

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

Double-checked this code.

  1. destination.tile->blocked check is actually redundant - we can't move onto tiles with obstacles anyway
  2. There is already a separate check for visitable objects 1 line below, so any visitable objects (blockvis or not) will be viewed as blocked

So looks correct.

Also checked all possible cases I could think of - tile guarded, blocked, visitable, etc (including presence of town). All work as intended.

Will clarify comments in code.

@IvanSavenko IvanSavenko changed the title Pathfinder fixes [1.5.2] Pathfinder fixes May 29, 2024
@IvanSavenko IvanSavenko merged commit dd2d0e2 into vcmi:beta May 29, 2024
14 checks passed
@IvanSavenko IvanSavenko deleted the pathfinder_fixes branch May 29, 2024 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nullkiller nullkiller nullkiller left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

hero is unable to move
2 participants
@IvanSavenko @nullkiller