Differential D1855

Move the Vertex Pathfinder to its own class
ClosedPublic
Actions

Authored by wraitii on May 1 2019, 10:52 AM.

Details

Reviewers

None

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP22253: Move the Vertex Pathfinder to its own helper class

Summary

The vertex pathfinder was still only a separate file from the Pathfinder component, unlike LongPathfinder which got its own class a long time ago.

This moves it to its own helper VertexPathfinder, which gets us ready for D14. Some struct definitions need to be moved around.
I think CCmpPathfinder_Common.h will be mergeable with CCmpPathfinder.cpp (or just renamed) then.

(D14 outtake).

Test Plan

Review changes

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.May 1 2019, 10:52 AM

wraitii added a parent revision: D1854: Remove 'Async' from short/long path requests names.

wraitii added a child revision: D14: Thread the pathfinder computations.

wraitii mentioned this in D14: Thread the pathfinder computations.

Build failure - The Moirai have given mortals hearts that can endure.

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

Harbormaster failed remote builds in B7337: Diff 7890!May 1 2019, 11:27 AM

Cleaned up of the Async renaming as this is going to be needed for more upstream revisions and I want to commit it.

source/simulation2/components/CCmpPathfinder_Common.h
69 ↗	(On Diff #7926)	Declaring the destructor here and having a default interface in the cpp file is necessary for std::unique_ptr to work, since otherwise the destructor tries to use an incomplete type and things don't work. Bit annoying, but I think the forward declarations are worth it.

Build failure - The Moirai have given mortals hearts that can endure.

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpPathfinder.h
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/simulation2/components/CCmpPathfinder_Common.h
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/simulation2/helpers/Pathfinding.h
|   1| /*·Copyright·(C)·2016·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2016"

source/simulation2/helpers/VertexPathfinder.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2017" year instead of "2019"

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

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

Harbormaster failed remote builds in B7380: Diff 7926!May 8 2019, 12:30 PM

Stan added a subscriber: Stan.May 8 2019, 12:56 PM

Stan added inline comments.

source/simulation2/components/CCmpPathfinder.cpp
202 ↗	(On Diff #7926)	Why not split this into two functions ? :)
source/simulation2/helpers/Pathfinding.h
32 ↗	(On Diff #7926)	Why the async prefix, can't they be used a non async calls ?
source/simulation2/helpers/VertexPathfinder.cpp
853 ↗	(On Diff #7926)	Safe to use a float here ?
865 ↗	(On Diff #7926)	Can't this be made an inline function ?
887 ↗	(On Diff #7926)	static_cast
935 ↗	(On Diff #7926)	Edges ?
940 ↗	(On Diff #7926)	This will trigger warnings for unreachable code.
source/simulation2/helpers/VertexPathfinder.h
48 ↗	(On Diff #7926)	Dunno if we have a convention for multiple declaration on the same lines. it seems it's not done in the struct above though.
51 ↗	(On Diff #7926)	Doxygen ?
56 ↗	(On Diff #7926)	Same here

Fix tests failing compilation.

source/simulation2/components/CCmpPathfinder.cpp
202 ↗	(On Diff #7926)	I don't want to introduce too many changes with this diff as I have another that decouples long and hierarchical pathfinder coming up. Cleanup can be done in another diff imo.
source/simulation2/helpers/Pathfinding.h
32 ↗	(On Diff #7926)	See D1854
source/simulation2/helpers/VertexPathfinder.cpp
853 ↗	(On Diff #7926)	debug code so yes
865 ↗	(On Diff #7926)	As with most things, it could. Again, I'm only trying to move code here.
935 ↗	(On Diff #7926)	probably good.
940 ↗	(On Diff #7926)	Will comment out again then.
source/simulation2/helpers/VertexPathfinder.h
56 ↗	(On Diff #7926)	All of the above >> Cleanup will be done later

Silier added a subscriber: Silier.May 8 2019, 1:15 PM

Silier added inline comments.

source/simulation2/components/CCmpPathfinder.cpp
1 ↗	(On Diff #7929)	2019 :)
source/simulation2/components/CCmpPathfinder_Common.h
1 ↗	(On Diff #7929)	2019 :)
source/simulation2/components/ICmpPathfinder.h
1 ↗	(On Diff #7929)	2019 :)

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

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpPathfinder.h
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/simulation2/components/CCmpPathfinder_Common.h
|   1| /*·Copyright·(C)·2017·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2017"

source/simulation2/helpers/VertexPathfinder.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2017" year instead of "2019"

source/simulation2/helpers/Pathfinding.h
|   1| /*·Copyright·(C)·2016·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2016"

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

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

Harbormaster completed remote builds in B7383: Diff 7929.May 8 2019, 1:32 PM

This revision was not accepted when it landed; it landed in state Needs Review.May 8 2019, 1:53 PM

Closed by commit rP22253: Move the Vertex Pathfinder to its own helper class (authored by wraitii). · Explain Why

This revision was automatically updated to reflect the committed changes.