Skip to content

Conversation

@Mauller
Copy link

@Mauller Mauller commented Sep 22, 2025

Merge with rebase

Part 2 in this epic series of munkee magic.

This PR cleans up various functions within the pathfinding, the goal to improve readability where possible and to improve performance where possible.

I have broken each function down into it's own commit to aid review.

This PR covers the functions

  • Pathfinder::internalFindPath
  • Pathfinder::chooseBestLocomotorForPosition
  • Pathfinder::findAttackPath
  • Pathfinder::getMoveAwayFromPath

A few of the refactors cover code simplification by inverting the logic and returning early or by simplifying functionality.


Todo

  • Replicate in generals
  • Test performance improvements, if any.

@Mauller Mauller self-assigned this Sep 22, 2025
@Mauller Mauller added Gen Relates to Generals ZH Relates to Zero Hour Refactor Edits the code with insignificant behavior changes, is never user facing labels Sep 22, 2025
Copy link

@xezon xezon left a comment

Choose a reason for hiding this comment

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

I am not a fan of merging multiple conditions into a single conditions. It loses the ability to put breakpoints in the debugger. I suggest to preserve the EA condition style because that helps debugging.

@Mauller
Copy link
Author

Mauller commented Sep 23, 2025

I am not a fan of merging multiple conditions into a single conditions. It loses the ability to put breakpoints in the debugger. I suggest to preserve the EA condition style because that helps debugging.

It's the only way to gain extra performance while maintaining retail compatibility, this code is pretty performance critical to the game and every extra if statement is a new possible branch miss and cache flush.

Combining conditionals that produce the same result has significantly less overhead than checking every one individually.

@xezon
Copy link

xezon commented Sep 23, 2025

Combining conditionals that produce the same result has significantly less overhead than checking every one individually.

Can you proof this with godbolt please?

@OmniBlade
Copy link

I have to second @xezon on that one, in my experience reversing these games, combining conditionals doesn't really decrease the number of comparisons as each condition still has to be jumped to and evaluated as part of the combined statement.

@Mauller
Copy link
Author

Mauller commented Sep 23, 2025

I have to second @xezon on that one, in my experience reversing these games, combining conditionals doesn't really decrease the number of comparisons as each condition still has to be jumped to and evaluated as part of the combined statement.

i might have been thinking of a different situation, i was sure i had read around that combining conditionals reduced branching.

I will revert some of the combining of conditionals and make them clearer to understand.

@Mauller Mauller force-pushed the refactor-pathfinding-simplify-code-2 branch from 0687642 to 3a9d92c Compare September 23, 2025 18:24
@Mauller
Copy link
Author

Mauller commented Sep 23, 2025

Tweaked based on feedback

@Mauller Mauller force-pushed the refactor-pathfinding-simplify-code-2 branch from 3a9d92c to ae7cfaf Compare September 26, 2025 14:49
@Mauller
Copy link
Author

Mauller commented Sep 26, 2025

That ones all tweaked based on feedback

@Mauller Mauller force-pushed the refactor-pathfinding-simplify-code-2 branch 2 times, most recently from 11bfe57 to 21dad4d Compare September 26, 2025 15:27
@xezon
Copy link

xezon commented Sep 27, 2025

Needs to be replicated to Generals

@Mauller Mauller changed the title refactor(pathfinding): cleanup functions to improve readability and reduce branching when possible - Part 2 refactor(pathfinding): simplify functions to improve readability - Part 2 Sep 28, 2025
@Mauller
Copy link
Author

Mauller commented Sep 28, 2025

will replicate across to generals soon, theres some further tweaks that i can do which requires part 1 to be merged first since it adds the checkCellOutsideExtents function that can be applied to some of the functions in this PR.

@Mauller Mauller force-pushed the refactor-pathfinding-simplify-code-2 branch from 21dad4d to 43074a8 Compare September 30, 2025 19:51
@Mauller
Copy link
Author

Mauller commented Sep 30, 2025

rebase with main before changes

@Mauller
Copy link
Author

Mauller commented Sep 30, 2025

Minor difference in ValidLocomotorSurfacesForCellType in generals, it doesn't have PathfindCell::CELL_BRIDGE_IMPASSABLE

@Mauller Mauller force-pushed the refactor-pathfinding-simplify-code-2 branch from 43074a8 to 6892d82 Compare September 30, 2025 20:37
@Mauller
Copy link
Author

Mauller commented Sep 30, 2025

Should be good now

@xezon xezon merged commit 487e5ce into TheSuperHackers:main Oct 1, 2025
20 checks passed
xezon pushed a commit that referenced this pull request Oct 1, 2025
xezon pushed a commit that referenced this pull request Oct 1, 2025
xezon pushed a commit that referenced this pull request Oct 1, 2025
@xezon xezon deleted the refactor-pathfinding-simplify-code-2 branch October 1, 2025 17:53
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Nov 10, 2025
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Nov 10, 2025
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Nov 10, 2025
fbraz3 pushed a commit to fbraz3/GeneralsX that referenced this pull request Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Gen Relates to Generals Refactor Edits the code with insignificant behavior changes, is never user facing ZH Relates to Zero Hour

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants