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

feat: Artificial Brilliance 0.1 - Dijikstra Maps #6069

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

DeltaEpsilon7787
Copy link
Contributor

@DeltaEpsilon7787 DeltaEpsilon7787 commented Feb 9, 2025

Purpose of change (The Why)

Current A* pathfinding is trash.

  1. It's one-to-one pathfinding system that does not utilize the fact that the vast majority of pathfinding done targets the same location within a turn.
  2. It's VERY old code and it shows in how spaghetti it has become
  3. Implementing any sort of potential fields is maddening due to how the algorithm is written
  4. A* progression is pretty hard to introspect into.
  5. Not a lot of knobs to turn, pathfinding is very same-y across runs, leading to congested routes, uninteresting pathing etc.
  6. It doesn't actually work because of max_length being so low, 99% of the time it never reaches the target.

Describe the solution (The How)

Dijikstra maps.
A significantly more aggressive pathfinding system for AI to use that will be necessary for more complicated AI behaviors related to pathing around obstacles and dangers to be done after this system is implemented.
Dijikstra maps allow many-to-1 pathfinding, i.e. from any position to some specific target. This is our most common case objectively, many zombies pathing towards the player.
I have implemented almost everything current pathfinding can do along with a number of additional features to be given JSON flags at a later point.

Feature list:

  • Consider terrain/furniture move cost when pathing
  • Adjustable costs for various terrain features
  • Recent death avoidance: preferably route around recently spawned corpses of same species
  • Live mob avoidance: pathing around other mobs
  • Suboptimal pathing: not always choosing the shortest paths available
  • Adjustable heurestic coeff: change from raw dijikstra to A*-like heurestics
  • Pathfinding area of search limits: only search within specific radius from target, only search within a certain angle from ourselves to target, ignore paths that are too long step-wise or too long cost-wise.
  • Support for flying mobs
  • Support for non-euclidean maps (mostly related to stairs that teleport you elsewhere)

Testing

To be done.

Additional context

To be done.

Checklist

Mandatory

@github-actions github-actions bot added the src changes related to source code. label Feb 9, 2025
Copy link
Contributor

autofix-ci bot commented Feb 9, 2025

Autofix has formatted code style violation in this PR.

I edit commits locally (e.g: git, github desktop) and want to keep autofix
  1. Run git pull. this will merge the automated commit into your local copy of the PR branch.
  2. Continue working.
I do not want the automated commit
  1. Format your code locally, then commit it.
  2. Run git push --force to force push your branch. This will overwrite the automated commit on remote with your local one.
  3. Continue working.

If you don't do this, your following commits will be based on the old commit, and cause MERGE CONFLICT.

@scarf005 scarf005 marked this pull request as draft February 9, 2025 01:56
std::optional<std::vector<tripoint>> DijikstraPathfinding::get_route( const tripoint from,
const RouteSettings route_settings )
{
if( !g->m.inbounds( from ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if( !g->m.inbounds( from ) ) {
if( !get_map().inbounds( from ) ) {

g brings in whole game.h as dependency, so get_* variants are preferred

Comment on lines 47 to 51
PathfindingSettings() = default;
PathfindingSettings( PathfindingSettings const & ) = default;

bool operator==( const PathfindingSettings &rhs ) const = default;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
PathfindingSettings() = default;
PathfindingSettings( PathfindingSettings const & ) = default;
bool operator==( const PathfindingSettings &rhs ) const = default;
auto operator<=>( const PathfindingSettings &rhs ) const = default;

removing default constructors would let consumers use more sugary syntax:

const auto path_settings = PathfindingSettings{
    .bash_strength = bash_skill(),
    .mob_presence_penalty = 10.0,
};

Comment on lines 170 to 172
RouteSettings() = default;
RouteSettings( RouteSettings const & ) = default;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RouteSettings() = default;
RouteSettings( RouteSettings const & ) = default;

same as here:

const auto route_settings = RouteSettings{
    .h_coeff = 1.0,
    .search_radius_coeff = 1.2,
    .search_cone_angle = 60.0,
    .alpha = 1.0,
};

Comment on lines 177 to 178

GraphPortal( const tripoint from, const float from_cost ) : from( from ), from_cost( from_cost ) {};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
GraphPortal( const tripoint from, const float from_cost ) : from( from ), from_cost( from_cost ) {};

also unneeded

@Coolthulhu
Copy link
Member

Current A* pathfinding is trash.

Eh, it's not THAT bad...
It could certainly benefit from a couple of fine-tuned optimizations, like precomputing reachability sets, locating rectangles of identical movement cost and pathfinding on those, swamps (in 3D pathfinding, we can treat entire basements as swamps), but it's pretty solid for a generic, agnostic implementation.

It doesn't actually work because of max_length being so low, 99% of the time it never reaches the target.

That's mostly a game design problem rather than an algorithmic one. We could give zombies bigger brains and check how laggy would the game get.
Might be worth at least a "smart zombies" mod to test it out.

Dijikstra maps.

So that's the name of that thing.
I did actually implement fully functional Dijkstra maps for older DDA, it just didn't make it into the game because I couldn't solve the problem of "zombie pileup" and didn't have a solid justification for why should be it in the game. Also, Kevin didn't like the idea because he preferred dumber zombies.
Nowadays and in BN, it's a viable idea.

This is our most common case objectively, many zombies pathing towards the player.

True, but it's not the most costly case.
Many-to-many is common enough and it should keep its A* for non-player targets - pure Dijkstra is significantly slower than A*, so it's only viable when we know there are many "pathers" to a given destination.
I didn't check your heuristic optimizations yet, it might be good enough with those.

Recent death avoidance

Wait with it for later PRs, it's a feature that needs careful balancing to be useful. This kind of balancing can hold up a PR for a relatively long time.

Support for non-euclidean maps

Stair teleporting rears its ugly head again... It's OK if there are minor pathing bugs related to tele-stairs, as long as they don't allow monsters to phase through floors or teleport without stairs. Bugs like subpar paths.
Stair teleport is an ancient bit of design debt that we're (slowly) trying to remove. In the future, all stairs will be aligned.

if( obstacle_part >= 0 ) {
int dummy;
const bool part_is_door = cur_vehicle->part_flag( obstacle_part, VPFLAG_OPENABLE );
const bool part_opens_from_inside = cur_vehicle->part_flag( obstacle_part, "OPENCLOSE_INSIDE" );
Copy link
Member

Choose a reason for hiding this comment

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

If it's used for pathfinding, it would be good to define a VPFLAG for it and use that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in old algo too, yes. There is no vpflag for this already existing seems like.

Copy link
Member

Choose a reason for hiding this comment

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

It's not necessary, it's just an optimization. Integer flags are faster to check than full inline string checks.

obstacle_bypass_cost = this->settings.door_open_cost;
} else if( can_bash ) {
float hp = cur_vehicle->cpart( obstacle_part ).hp();
obstacle_bypass_cost = this->settings.bash_cost * hp / this->settings.bash_strength;
Copy link
Member

Choose a reason for hiding this comment

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

Vehicle bashing is not that simple, because vehicle parts have this thing called "damage threshold". It significantly affects zombies pathing through heavy-duty vehicles.
You can copy the old implementation for now, though it is outdated - vehicle parts now also have damage reduction, which the old implementation doesn't take into account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd really love to remove the whole logic out of pathfinding and implement an estimate of bashes-to-destroy in vehicle.cpp at this point

Copy link
Member

Choose a reason for hiding this comment

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

Just don't make it a vehicle method. It should be in some namespace, either bashing (would go into map.h/.cpp) or something vehicle-related that would go into vehicle.h/.cpp, but I can't think of a good namespace name for it.

The formula for expected number of bashes is (according to ChatGPT):

double expected_events(int c_start, int x_min, int x_max, int t) {
    // If t > x_max, no outcome is valid.
    if (t > x_max) {
        return std::numeric_limits<double>::infinity();
    }
    
    // If t <= x_min, all outcomes are valid.
    if (t <= x_min) {
        return (2.0 * c_start) / (x_min + x_max);
    }
    
    // Otherwise, valid outcomes are those x in [t, x_max].
    // Total outcomes:
    int totalCount = x_max - x_min + 1;
    // Valid outcomes count:
    int validCount = x_max - t + 1;
    
    // Expected value when valid:
    double validAvg = (t + x_max) / 2.0;
    
    // Overall expected decrement per event:
    double E = (validCount * validAvg) / totalCount;
    
    // Expected number of events needed:
    return c_start / E;  
}

Where c_start is starting hp, x_min/max is min and max value of bashing roll, and t is the damage threshold below which the damage is effectively 0. The formula assumes x is uniform.
I checked the formula for a couple of simple cases and it seems to be valid. Didn't check GPT's reasoning and derivation, was too lazy.

#include "rng.h"

const size_t DIJIKSTRA_ARRAY_SIZE = OVERMAP_LAYERS * MAPSIZE_Y * MAPSIZE_X;
typedef std::pair<float, tripoint> val_pair;
Copy link
Member

Choose a reason for hiding this comment

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

Would be much better to use integers if possible. Floats are highly likely to result in less predictable (in a bad way) paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really don't think so. We only care about number ordering, not numbers themselves. Integers make it vastly more annoying to do anything because they're discrete. Additionally, I'm using NAN and INF as magic sentinel values.

@DeltaEpsilon7787
Copy link
Contributor Author

DeltaEpsilon7787 commented Feb 12, 2025

That's mostly a game design problem rather than an algorithmic one. We could give zombies bigger brains and check how laggy would the game get.

The underlying reason why I wanted to implement dijikstra map was to implement potential fields actually, in other words make areas more or less attractive for movement. This was just unviable in legacy impl.

True, but it's not the most costly case.

It's repeated cost that can be reduced significantly.

I didn't check your heuristic optimizations yet, it might be good enough with those.

With h-coeff = 1.0, it is exactly equivalent to A* because what we get is g(n) + 1.0*h(n) as the cost function.

Wait with it for later PRs, it's a feature that needs careful balancing to be useful. This kind of balancing can hold up a PR for a relatively long time.

That's the intention anyway. I want to implement the framework (pathfinding) first, then a couple mods that enable this behavior in mobs.

const units::angle deviation = conic_angle - objective_angle;

return -max_cone_angle <= deviation && deviation <= max_cone_angle;
}
Copy link
Member

Choose a reason for hiding this comment

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

If this is called a lot, might want to check bounding box before doing actual cone checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't follow?

@DeltaEpsilon7787 DeltaEpsilon7787 force-pushed the artificial_brilliance_pathfinding branch from 84a057d to 3d6bb21 Compare February 13, 2025 08:21
@scarf005 scarf005 requested a review from Coolthulhu February 16, 2025 16:12
@scarf005 scarf005 changed the title [WIP] Artificial Brilliance 0.1: Dijikstra Maps feat: Artificial Brilliance 0.1 - Dijikstra Maps Feb 16, 2025
@DeltaEpsilon7787 DeltaEpsilon7787 force-pushed the artificial_brilliance_pathfinding branch from ec5b450 to f358ff8 Compare February 19, 2025 16:18
Copy link
Member

@chaosvolt chaosvolt left a comment

Choose a reason for hiding this comment

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

So, discussion in the BN server has revealed to me that an apparent side effect of this PR is it will make basic zombies act like they have the pathfinding abilities of actually intelligent enemies, which uh. Completely defeats the point of pathfinding setting JSON, and makes it so that zombies ignore obstacles and walls when their entire point is being numerous but dumber than more threatening enemies.

2025-02-21_17-07-57.mp4

This sort of psychic nonsense, where the zombies realized they had to go off the beaten path to find the stairs, and then double back to bash through the door, is COMPLETELY excessive.

We want zombies to be able to find stairs yes since currently all monsters in general tend to get stuck on any given z-level, the problem this example demonstrated is the zeds were able to make a MASSIVE detour away from where they spotted the player just to find stairs they couldn't even see from outside, and then continue to home in on a player they have zero sightline on right through the only obvious weakpoint in the interfering terrain.

EDIT: Not actually implemented yet, so long as default behavior for zeds is still fairly short to no pathfinding distance we should be fine.

@chaosvolt
Copy link
Member

Further discussion revealed this isn't actually implemented yet.
image

Having it be an option the player can turn on is prolly fine, just so long as default behavior is no advanced pathfinding for normal zeds, or possible no more than 3-5 tiles to ensure that they can find stairs but not make journies like in the above video.

@chaosvolt chaosvolt dismissed their stale review February 21, 2025 18:26

panic-reaction to unimplemented WIP work

@OrenAudeles OrenAudeles self-requested a review February 21, 2025 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
src changes related to source code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants