Details
- Reviewers
vladislavbelov - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) - Commits
- rP23029: Cleanup CCmpRallyPointRenderer
Test it doesn't break anything
Report any possible improvements in that class we can include.
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
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/799/
Why separate header? I don't think it helps much. All other components don't have headers.
Build failure - The Moirai have given mortals hearts that can endure.
Link to build: https://jenkins.wildfiregames.com/job/differential/800/
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/801/
Also check that:
- A comment starts with a capital letter.
- No space before brace in static_cast<...>(.
| source/simulation2/components/CCmpRallyPointRenderer.cpp | ||
|---|---|---|
| 1 ↗ | (On Diff #7006) | Bump year. |
| 341 ↗ | (On Diff #7006) | I suppose we can replace it with range based loop. |
| 480 ↗ | (On Diff #7006) | There is an erasing of elements from this container, and std::deque is faster by constant for such operation. So I think it'd be better to use std::deque here, some performance tests may help to decide. |
| 593 ↗ | (On Diff #7006) | The for looks weird. I would prefer following: for (size_t i = 0; i < coords.size(); ++i) { // ... coords.erase(coords.rbegin() + i); } |
| 750 ↗ | (On Diff #7006) | const or constexpr. |
| 850 ↗ | (On Diff #7006) | Range based loop here and below. |
| 1290 ↗ | (On Diff #7006) | Please remove UB here (#5288). It should be segments.erase(std::prev(segments.end())); or segments.erase(segments.rbegin()); I suppose. |
| source/simulation2/components/CCmpRallyPointRenderer.h | ||
| 1 ↗ | (On Diff #7006) | Bump year. |
| 39 ↗ | (On Diff #7006) | Common sorted list of includes. |
| 73 ↗ | (On Diff #7006) | Please reorder the accessibility zones to: public, protected, private. And this one is private, because it's default for class. |
| 292 ↗ | (On Diff #7006) | Unnecessary. Also it reports about No newline at end of property. |
| source/simulation2/components/CCmpRallyPointRenderer.cpp | ||
|---|---|---|
| 593 ↗ | (On Diff #7006) | Or as the article suggests: segments.pop_back();. |
| source/simulation2/components/CCmpRallyPointRenderer.cpp | ||
|---|---|---|
| 480 ↗ | (On Diff #7006) | Definitely do performance tests, as it generally turns out std::vector is faster anyways. |
How am I suppose to rpofile that ? Create a new cpp project copy objects and fill and remove queues ?
In this situation given that it won't change the gameplay I would procure an MP replay, add Profiler2 profiling around affected functions, and replay once with vector and once with deque, then use the profiler2 interface to compare. There's a guide in trac on how to use Profiler2.
Fix above comments. Not sure yet how to use the profiler but I'm interested, unless you guys want to do it of course.
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/943/
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/944/
| source/simulation2/components/CCmpRallyPointRenderer.cpp | ||
|---|---|---|
| 391 ↗ | (On Diff #7307) | Unnecessary parenthesis. |
| 606 ↗ | (On Diff #7307) | You changed the logic here. |
| 634 ↗ | (On Diff #7307) | Here too. |
| 854 ↗ | (On Diff #7307) | segments.pop_back() was for this line. |
| 593 ↗ | (On Diff #7006) | This comment was to the UB line (I don't know how the comment points out to this line). |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/960/
@vladislavbelov Vector will definitely be better, https://baptiste-wicht.com/posts/2012/12/cpp-benchmark-vector-list-deque.html can I commit this now ? :)
| source/simulation2/components/CCmpRallyPointRenderer.cpp | ||
|---|---|---|
| 480 ↗ | (On Diff #7006) | |
If you want more specific output https://pastebin.com/myEAnKEj
9000000 samples
cl.exe fine.cpp /EHsc
STD: Vector 'push_back' time:1.62
STD: Deque 'push_back' time: 2.193
STD: Vector 'emplace_back' time: 1.551
STD: Deque 'emplace_back' time: 2.222
STD: Vector 'erase' time: 1.236
STD: Deque 'erase' time: 10.948
cl.exe fine.cpp /EHsc /O1
STD: Vector 'push_back' time:0.428
STD: Deque 'push_back' time: 0.771
STD: Vector 'emplace_back' time: 0.468
STD: Deque 'emplace_back' time: 0.689
STD: Vector 'erase' time: 0.074
STD: Deque 'erase' time: 1.025
cl.exe fine.cpp /EHsc /O2
STD: Vector 'push_back' time:0.297
STD: Deque 'push_back' time: 0.647
STD: Vector 'emplace_back' time: 0.341
STD: Deque 'emplace_back' time: 0.64
STD: Vector 'erase' time: 0.029
STD: Deque 'erase' time: 0.62
cl.exe fine.cpp /EHsc /OX
STD: Vector 'push_back' time:0.299
STD: Deque 'push_back' time: 0.669
STD: Vector 'emplace_back' time: 0.345
STD: Deque 'emplace_back' time: 0.641
STD: Vector 'erase' time: 0.027
STD: Deque 'erase' time: 0.628
| source/simulation2/components/CCmpRallyPointRenderer.cpp | ||
|---|---|---|
| 601 ↗ | (On Diff #7339) | We shouldn't use size_t for such kind of loop conditions: for (size_t i = coords.size() - 1; i >= 0; i--). Because subtraction from size_t that equals to 0 isn't good. You'll get underflow: in coords.size() - 1 if coords.size() == 0 and in i-- if i == 0. The last one means an infinity loop. |
| 739 ↗ | (On Diff #7339) | It seems it was the only one usage of the FixFootprintWaypoints, do we need it? Or why the code was removed? Is it useless? |
| source/simulation2/components/CCmpRallyPointRenderer.cpp | ||
|---|---|---|
| 601 ↗ | (On Diff #7339) | Just loop from coords.size() to a strict >0 and access coords[i-1]. |
| 739 ↗ | (On Diff #7339) | With the I am not sure what this footprint-related condition is about. I guess the code was maybe commented out instead of deleted because of that uncertainty. We should delete it now. |
| 884 ↗ | (On Diff #7339) | Usually, classinit, serialize, deserialize, message handling, and message subscriptions are up there just after Init. Easier to find them. |
| 1068 ↗ | (On Diff #7339) | The schema should also be in the top region of the file. |
| source/simulation2/components/CCmpRallyPointRenderer.cpp | ||
|---|---|---|
| 601 ↗ | (On Diff #7339) | I'd prefer to not have the erasing with alive indices. // It'd probably good to add a function for explicit Vector2D > CFixedVector2D conversion. while (!coords.empty() && Geometry::PointIsInSquare(CFixedVector2D(coords.back()) - center, u, v, halfSize)) { coords.pop_back(); } |
| source/simulation2/components/CCmpRallyPointRenderer.cpp | ||
|---|---|---|
| 601 ↗ | (On Diff #7339) | Ah, sure, I hadn't taken a look at the contents of the loop. It should be changed indeed. But in case Stan encounters another loop with a size_t and descending order, this is the way to avoid the underflow. |
- Fix whitespaces reported by git.
- Remove FixFootprintWaypoints/ - Fix broken loop.
- Move classinit, serialize, deserialize, message handling, and message subscriptions, init, getschema at the top of the file.
| source/simulation2/components/CCmpRallyPointRenderer.cpp | ||
|---|---|---|
| 601 ↗ | (On Diff #7339) | Thanks for the tips guys ! Since the function is gone now it's all good. |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1153/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1156/display/redirect
I tested it on VS2015, it works for me. The movement looks ok, the old code has some issues, but it'd be better to fix them in a separate diff.
| source/simulation2/components/CCmpRallyPointRenderer.h | ||
|---|---|---|
| 21 ↗ | (On Diff #7654) | I think we have an empty line after paired header. |
| 39 ↗ | (On Diff #7006) | Still not sorted. But maybe in a separate commit. |