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 6787
Build 11161: 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.

339–340

I suppose we can replace it with range based loop.

480–481

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.

601

The for looks weird. I would prefer following:

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

const or constexpr.

871

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
601

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
480–481

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
391

Unnecessary parenthesis.

601

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

606

You changed the logic here.

634

Here too.

854

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/