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 a variant of closest_points_first that takes a predicate #75961

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

Conversation

CLIDragon
Copy link
Contributor

Summary

None

Purpose of change

Part of #75945. The variant will be used in pathfinding to find the nearest ramp or stairs without having to enumerate all points.

Describe the solution

Adds a variant of closest_points_first, find_closest_points_first that takes a predicate function. It is generic over point and tripoint, thereby avoiding a double allocation in tripoint variants.

Describe alternatives you've considered

The bounds check could be skipped by adding an inbounds variant, but this is certainly too much work to skip three checks.

Using std::variant instead of typename Point. I'm not particularly biased towards one way or another.

Testing

As long as current closest_points_first tests pass, this will have been implemented correctly.

Additional context

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` new contributor astyled astyled PR, label is assigned by github actions json-styled JSON lint passed, label assigned by github actions NPC / Factions NPCs, AI, Speech, Factions, Ownership Vehicles Vehicles, parts, mechanics & interactions labels Aug 26, 2024
@CLIDragon
Copy link
Contributor Author

I'm not sure why this is failing. Building succeeds on my machine, and I've included point.h in all of the referenced files.

@CLIDragon CLIDragon force-pushed the closest_points_first branch from 6747a37 to a40e8ac Compare August 30, 2024 12:54
src/monmove.cpp Outdated Show resolved Hide resolved
@CLIDragon CLIDragon force-pushed the closest_points_first branch 2 times, most recently from 878aded to 0dde0b1 Compare September 5, 2024 13:45
@CLIDragon
Copy link
Contributor Author

Build failures appear to be unrelated, though I would prefer to rerun to check.

@GuardianDll
Copy link
Member

sadly can't be fixed by rerun, #76349 need to be merged first

@Maleclypse
Copy link
Member

Can you resolve conflicts?

@CLIDragon CLIDragon marked this pull request as draft November 7, 2024 05:44
@CLIDragon CLIDragon force-pushed the closest_points_first branch from 44f5890 to 6abaf58 Compare November 7, 2024 06:22
@CLIDragon CLIDragon marked this pull request as ready for review November 7, 2024 06:26
@CLIDragon
Copy link
Contributor Author

Merge conflicts resolved.

@CLIDragon CLIDragon force-pushed the closest_points_first branch from 6abaf58 to e622774 Compare November 9, 2024 08:03
@github-actions github-actions bot added astyled astyled PR, label is assigned by github actions and removed astyled astyled PR, label is assigned by github actions labels Nov 9, 2024
@Maleclypse
Copy link
Member

Is clang angry from this PR or some other reason?

@CLIDragon
Copy link
Contributor Author

Is clang angry from this PR or some other reason?

Yes, it's this PR. I should probably mark as draft until I get around to fixing the issues.

@CLIDragon CLIDragon marked this pull request as draft November 11, 2024 04:12
@github-actions github-actions bot removed the astyled astyled PR, label is assigned by github actions label Nov 11, 2024
@CLIDragon CLIDragon marked this pull request as ready for review November 11, 2024 07:32
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Nov 11, 2024
src/point.h Outdated Show resolved Hide resolved
CLIDragon and others added 8 commits December 29, 2024 11:53
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@CLIDragon CLIDragon force-pushed the closest_points_first branch from 9d54406 to 3acdb28 Compare December 31, 2024 01:33
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 31, 2024
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 31, 2024
@CLIDragon CLIDragon closed this Dec 31, 2024
@CLIDragon CLIDragon reopened this Dec 31, 2024
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 1, 2025
@CLIDragon
Copy link
Contributor Author

This is failing on vehicle_split_test again and should be fine to merge.

@CLIDragon CLIDragon closed this Jan 1, 2025
@CLIDragon CLIDragon reopened this Jan 1, 2025
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jan 1, 2025
@CLIDragon CLIDragon closed this Jan 1, 2025
@CLIDragon CLIDragon reopened this Jan 1, 2025
@CLIDragon CLIDragon closed this Jan 3, 2025
@CLIDragon CLIDragon reopened this Jan 3, 2025
@CLIDragon CLIDragon closed this Jan 3, 2025
@CLIDragon CLIDragon reopened this Jan 3, 2025
@CLIDragon CLIDragon closed this Jan 4, 2025
@CLIDragon CLIDragon reopened this Jan 4, 2025
@CLIDragon CLIDragon marked this pull request as draft January 6, 2025 04:13
@CLIDragon CLIDragon marked this pull request as ready for review January 6, 2025 04:13
@GuardianDll
Copy link
Member

Please resolve branch conflicts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions new contributor NPC / Factions NPCs, AI, Speech, Factions, Ownership Vehicles Vehicles, parts, mechanics & interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants