Page MenuHomeWildfire Games

Cleanup RallyPointRenderer
Needs ReviewPublic

Authored by Stan on Nov 24 2018, 5:25 PM.

Details

Reviewers
vladislavbelov
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Summary

This is the first step for D557 before creating an abstract class.

Replace std::deque by std::vector
Replace some casts.
Includes D1681
Move the class and struct declaration to a separate file for clarity.

Test Plan

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
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 7087
Build 11570: Vulcan BuildJenkins

Event Timeline

Stan created this revision.Nov 24 2018, 5:25 PM
Vulcan added a subscriber: Vulcan.Nov 24 2018, 5:25 PM

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.

Stan added a comment.Nov 24 2018, 5:42 PM

Cause that file is huge. Also I think it's good practise to do so.

Stan updated this revision to Diff 7005.Nov 24 2018, 6:28 PM

Try to fix the build. Fix warnings.

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

Link to build: https://jenkins.wildfiregames.com/job/differential/800/

Stan updated this revision to Diff 7006.Nov 24 2018, 6:45 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/801/

wraitii added a reviewer: Restricted Owners Package.Dec 31 2018, 10:00 AM

Also check that:

  • A comment starts with a capital letter.
  • No space before brace in static_cast<...>(.
source/simulation2/components/CCmpRallyPointRenderer.cpp
1

Bump year.

485–486

I suppose we can replace it with range based loop.

627–628

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.

707–708

The for looks weird. I would prefer following:

for (size_t i = 0; i < coords.size(); ++i)
{
	// ...
	coords.erase(coords.rbegin() + i);
}
822

const or constexpr.

927

Range based loop here and below.

1295

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
2

Bump year.

40

Common sorted list of includes.

74

Please reorder the accessibility zones to: public, protected, private. And this one is private, because it's default for class.

293

Unnecessary. Also it reports about No newline at end of property.

vladislavbelov added inline comments.Jan 7 2019, 1:45 AM
source/simulation2/components/CCmpRallyPointRenderer.cpp
707–708

Or as the article suggests: segments.pop_back();.

wraitii added a subscriber: wraitii.Jan 7 2019, 9:43 AM
wraitii added inline comments.
source/simulation2/components/CCmpRallyPointRenderer.cpp
627–628

Definitely do performance tests, as it generally turns out std::vector is faster anyways.

Stan added a comment.Jan 7 2019, 11:30 AM

How am I suppose to rpofile that ? Create a new cpp project copy objects and fill and remove queues ?

Stan planned changes to this revision.Jan 7 2019, 11:38 AM
wraitii added a comment.EditedJan 7 2019, 1:42 PM

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.

Stan updated this revision to Diff 7306.Jan 8 2019, 12:46 AM

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/

Stan updated this revision to Diff 7307.Jan 8 2019, 12:48 AM
Stan marked 9 inline comments as done.

Fix indent.

Vulcan added a comment.Jan 8 2019, 1:16 AM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/944/

vladislavbelov added inline comments.Jan 13 2019, 3:30 PM
source/simulation2/components/CCmpRallyPointRenderer.cpp
538

Unnecessary parenthesis.

707–708

This comment was to the UB line (I don't know how the comment points out to this line).

707–708

You changed the logic here.

707–708

Here too.

910

segments.pop_back() was for this line.

Stan updated this revision to Diff 7339.Jan 13 2019, 6:46 PM
Stan marked 6 inline comments as done.

Fix @vladislavbelov's comments.

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

Link to build: https://jenkins.wildfiregames.com/job/differential/960/

Stan marked an inline comment as done.EditedMar 6 2019, 3:50 PM

@vladislavbelov Vector will definitely be better, https://baptiste-wicht.com/posts/2012/12/cpp-benchmark-vector-list-deque.html can I commit this now ? :)

Stan marked 2 inline comments as done.Mar 6 2019, 3:50 PM
Stan added inline comments.
Stan marked an inline comment as done.Mar 6 2019, 4:25 PM

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

vladislavbelov requested changes to this revision.Apr 3 2019, 3:06 AM
vladislavbelov added inline comments.
source/simulation2/components/CCmpRallyPointRenderer.cpp
707–708

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

It seems it was the only one usage of the FixFootprintWaypoints, do we need it? Or why the code was removed? Is it useless?

This revision now requires changes to proceed.Apr 3 2019, 3:06 AM
Stan added a subscriber: Itms.Apr 3 2019, 8:09 AM
Stan added inline comments.
source/simulation2/components/CCmpRallyPointRenderer.cpp
707–708

Is there a better way to write this loop than for(int i = static_cast<int>)(coords.size() - 1); i >= 0; i--)

739

The code was removed because of the new Pathfinder in rP16751 maybe @Itms know more about it.

Itms added inline comments.Apr 3 2019, 9:29 AM
source/simulation2/components/CCmpRallyPointRenderer.cpp
707–708

Just loop from coords.size() to a strict >0 and access coords[i-1].

739

With the new modern pathfinder, the paths are already free of obstructed waypoints. So this step doesn't look necessary indeed.

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.

940

Usually, classinit, serialize, deserialize, message handling, and message subscriptions are up there just after Init. Easier to find them.

1135

The schema should also be in the top region of the file.

vladislavbelov added inline comments.Apr 3 2019, 10:55 AM
source/simulation2/components/CCmpRallyPointRenderer.cpp
707–708

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();
}
Itms added inline comments.Apr 3 2019, 11:10 AM
source/simulation2/components/CCmpRallyPointRenderer.cpp
707–708

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.

Stan updated this revision to Diff 7650.Apr 3 2019, 11:20 AM
Stan marked 5 inline comments as done.
  • 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.
Stan marked an inline comment as done.Apr 3 2019, 11:21 AM
Stan added inline comments.
source/simulation2/components/CCmpRallyPointRenderer.cpp
707–708

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

vladislavbelov added inline comments.Apr 3 2019, 1:55 PM
source/simulation2/components/CCmpRallyPointRenderer.cpp
437

const CVector2D& point?

source/simulation2/components/CCmpRallyPointRenderer.h
26

I suppose we don't need this empty line and one below.

279
#endif // INCLUDED_CCMPRALLYPOINTRENDERER
Stan updated this revision to Diff 7654.EditedApr 3 2019, 3:44 PM
Stan marked 4 inline comments as done.

Fix the three above comments.

Vulcan added a comment.Apr 3 2019, 3:49 PM

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

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