Page MenuHomeWildfire Games

Const-correct the hierarchical pathfinder (D53 outtake)
ClosedPublic

Authored by wraitii on Apr 19 2019, 5:55 PM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22210: Const-correct the hierarchical pathfinder.
Summary

This makes the hierarchical-pathfinder const-correct, enabling D1491 (and then D14).

I prefer using .at() here over find() as it makes the code neater and the performance impact is probably negligible.

Test Plan

Compile.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

wraitii created this revision.Apr 19 2019, 5:55 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/helpers/HierarchicalPathfinder.cpp
|   1| /*·Copyright·(C)·2016·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2016"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1230/display/redirect

elexis added a subscriber: elexis.Apr 19 2019, 9:20 PM

const-correct

(I guess the previous code was not incorrect, but adding the const adds benefits)

I prefer using .at() here over find() as it makes the code neater and the performance impact is probably negligible.

That's at least an equally important (if not greater) change than adding the const.

The array access inserts an element, but .at throws a std::out_of_range if the element does not exist.
http://www.cplusplus.com/reference/map/map/operator[]/
https://en.cppreference.com/w/cpp/container/map/at
(I guess the elements exist, didn't check further)

Otherwise looks harmless.

Yup, and I think I've actually messed it up but I'll check with an upstream diff.

source/simulation2/helpers/HierarchicalPathfinder.cpp
564 ↗(On Diff #7774)

safe as passClass _should_ always exist (or the code is doing something stupid anyways)

714 ↗(On Diff #7774)

I think this one can crash actually, because there might be no edge to the current region. I might need to change it back. At least it crashes with one of my upstream "outtake" patches (not yet a diff for it) so I'll check.

The changes from D53 ensure this doesn't happen so I'll need to back port them here I think: https://code.wildfiregames.com/D53#inline-35820

725 ↗(On Diff #7774)

safe as passClass _should_ always exist (or the code is doing something stupid anyways)

740 ↗(On Diff #7774)

safe as passClass _should_ always exist (or the code is doing something stupid anyways).
The second is a vector access so it remains [].

wraitii planned changes to this revision.Apr 19 2019, 10:21 PM
wraitii updated this revision to Diff 7777.Apr 20 2019, 8:49 AM

Fix FindReachableRegions, broken from using at()

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/helpers/HierarchicalPathfinder.cpp
|   1| /*·Copyright·(C)·2016·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2016"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1233/display/redirect

It's probably safer to check whether the key exists (.contains) before possibly running into an exception, and it might be nicer to insert the keys explicitly rather than having them automatically be inserted when trying to read from them (at least if in most cases the key already exists).

source/simulation2/helpers/HierarchicalPathfinder.cpp
708 ↗(On Diff #7777)

.contains?

In D1830#75252, @elexis wrote:

It's probably safer to check whether the key exists (.contains) before possibly running into an exception, and it might be nicer to insert the keys explicitly rather than having them automatically be inserted when trying to read from them (at least if in most cases the key already exists).

.contains is C++20 (though I agree). I don't understand the rest of your message? If the initial region has no edges, it has no neighbours, so we can return early from FindReachableRegions. There's no automatic insertion here?

This is next on my commit list (once builds have passed for the rest) as it is needed for D1832 to cleanly apply, and I'll push new tests proving D1832 then, then commit that, which enables running D1834 on top of all these tests, which I then feel reasonably confident to commit soon (I'd like to run an MP replay with it to ensure hashes are consistent though).

This revision was not accepted when it landed; it landed in state Needs Review.Apr 22 2019, 6:07 PM
This revision was automatically updated to reflect the committed changes.