Details
- Reviewers
Kuba386 - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP22902: Move path computations to an actual worker to prepare for threading.
Compile, check that things still work.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 8508 Build 13915: Vulcan Build Jenkins Build 13914: arc lint + arc unit
Event Timeline
source/simulation2/Simulation2.cpp | ||
---|---|---|
597 | Should't a boolean be passed ? | |
source/simulation2/components/CCmpPathfinder.cpp | ||
100 | could it be a shared_pointer ? @vladislavbelov said we should avoid raw pointers but I'm not sure when we should use those. |
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/1503/display/redirect
source/simulation2/components/CCmpPathfinder.cpp | ||
---|---|---|
100 | Here is what Herb Sutter has to say about it:
Source: |
source/simulation2/components/CCmpPathfinder.cpp | ||
---|---|---|
100 | Here I'm just initialising the unique pointer, so using new is legitimate. |
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/Simulation2.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/1510/display/redirect
source/simulation2/Simulation2.cpp | ||
---|---|---|
597 | Yes, shouldn't it be false? |
source/simulation2/Simulation2.cpp | ||
---|---|---|
597 | I'm not sure what Stan meant by that or you you mean by that? |
source/simulation2/Simulation2.cpp | ||
---|---|---|
597 | I mean shouldn't we change: StartProcessingMoves(); -> StartProcessingMoves(false); or void StartProcessingMoves(bool useMax) { .... } -> void StartProcessingMoves(bool useMax = false) { .... } ? |
source/simulation2/Simulation2.cpp | ||
---|---|---|
597 | = false is defined in ICmpPathfinder.h |
Fix a silly mistake that made StartProcessingMoves not actually process moves when using the 'rate limiter'.
Rebased. RC.
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/148/display/redirect
Have m_Workers from the start as a vector of only one item. Don't use unique_ptr.
RC still.
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/components/CCmpPathfinder_Common.h | 34| #include·"graphics/Overlay.h" | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'template<...' is invalid C code. Use --std or --language to configure the language. source/simulation2/components/tests/test_Pathfinder.h | 32| class·TestCmpPathfinder·:·public·CxxTest::TestSuite | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classTestCmpPathfinder:' is invalid C code. Use --std or --language to configure the language. source/simulation2/components/ICmpPathfinder.h | 34| template<typename·T>·class·Grid; | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'template<...' is invalid C code. Use --std or --language to configure the language. source/simulation2/Simulation2.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/docker-differential/151/display/redirect
Don't actually check for sorting of paths because the long/short paths are handled separately and it won't work.
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/components/ICmpPathfinder.h | 34| template<typename·T>·class·Grid; | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'template<...' is invalid C code. Use --std or --language to configure the language. source/simulation2/components/tests/test_Pathfinder.h | 32| class·TestCmpPathfinder·:·public·CxxTest::TestSuite | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classTestCmpPathfinder:' is invalid C code. Use --std or --language to configure the language. source/simulation2/components/CCmpPathfinder_Common.h | 34| #include·"graphics/Overlay.h" | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'template<...' is invalid C code. Use --std or --language to configure the language. source/simulation2/Simulation2.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/docker-differential/175/display/redirect
Tested on Jebel Barkal, Mainland, Lake, Snowflake, with AI and multiplayer.
Everything seems to work.
(m_SameTurnMovesCount comes from rP9665)
source/simulation2/Simulation2.cpp | ||
---|---|---|
597 | Why not pass it explicitly, so that the reader of the cpp file is indicated that there is an option to be considered here? What was the purpose of that max from the cited commit, and why does this purpose not apply anymore for threads? | |
source/simulation2/components/CCmpPathfinder.cpp | ||
100 | The magic of emplace is that it forwards the arguments to the constructor. m_Workers.emplace_back(PathfinderWorker{}); -> m_Workers.emplace_back(); | |
712 | Should yield linker errors if the specialization is not implemented, so no need to make this a runtime error then? | |
731 | m_Results.push_back({req.ticket, req.notify, path}); I think it's equivalent code flow, but the first one relies on copy elision, the second one is ensured to construct in place without copy or move assignment / constructor. | |
733 | (perhaps one can avoid back() pop_back() with an erase idiom, but that sounds perverse) | |
740 | (same) | |
756 | ComputeShortPathAsync(\n \t... \t...) { ? | |
765 | (wondering whether its faster to pass a point with 2 components by ref or those two numbers, probably the latter) | |
769 | Should check with clang that these proxies are inlined:
(Gotta save the log and grep for function name or file + line nr) | |
798 | (copy elision should hold, may verify with clang) |
source/simulation2/Simulation2.cpp | ||
---|---|---|
597 | This last call should compute all moves, since it's the last call before the next turn. So obviously it can't use the limit. It already wasn't using the limit before, nothing's changed. Nothing against explicitly passing the argument, don't care. | |
source/simulation2/components/CCmpPathfinder.cpp | ||
712 | ??? | |
756 | not sure how we indent in such cases. It's aligned on the parenthesis here. | |
769 | Definitely completely irrelevant, performance wise, either way, but if one wants to, sure. |
source/simulation2/components/CCmpPathfinder.cpp | ||
---|---|---|
712 | I think an explicit static error is better than a random linker error. |
source/simulation2/components/CCmpPathfinder.cpp | ||
---|---|---|
712 | I didn't know that command yet, it's at compile time, so that's indeed even better than later at linking! |
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/673/display/redirect
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/163/display/redirect
And with that I remember why I chose push_back, I need to add a constructor for emplace.
Add a default default constructor so that the type remains trivial
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/164/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/simulation2/components/tests/test_Pathfinder.h | 32| class·TestCmpPathfinder·:·public·CxxTest::TestSuite | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'classTestCmpPathfinder:' is invalid C code. Use --std or --language to configure the language. source/simulation2/components/ICmpPathfinder.h | 34| template<typename·T>·class·Grid; | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'template<...' is invalid C code. Use --std or --language to configure the language. source/simulation2/components/CCmpPathfinder_Common.h | 34| #include·"graphics/Overlay.h" | | [MAJOR] CPPCheckBear (syntaxError): | | Code 'template<...' is invalid C code. Use --std or --language to configure the language. Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/674/display/redirect