Finds a Character based on distance and health Planned maintenance scheduled April 23, 2019 at 23:30 UTC (7:30pm US/Eastern) Announcing the arrival of Valued Associate #679: Cesar Manara Unicorn Meta Zoo #1: Why another podcast?Euclidian distance - optimization and castingSpeed, distance and time calculatorFast Document Distance, C++Pairwise distance and residual calculationSplit a character string based on change of characterDistance and force calculation for a molecular dynamics simulationOpenMP SIMD Euclidean DistanceCalculating Euclidean distanceAlgorithm that parses through input of points and finds distanceReads a inFile and grades tests scores and finds max, min, and avg
Did any compiler fully use 80-bit floating point?
Universal covering space of the real projective line?
Why datecode is SO IMPORTANT to chip manufacturers?
Nose gear failure in single prop aircraft: belly landing or nose-gear up landing?
Where is the Next Backup Size entry on iOS 12?
Is there hard evidence that the grant peer review system performs significantly better than random?
Weaponising the Grasp-at-a-Distance spell
In musical terms, what properties are varied by the human voice to produce different words / syllables?
Co-worker has annoying ringtone
Can an iPhone 7 be made to function as a NFC Tag?
The test team as an enemy of development? And how can this be avoided?
What are the main differences between Stargate SG-1 cuts?
Putting class ranking in CV, but against dept guidelines
My mentor says to set image to Fine instead of RAW — how is this different from JPG?
Project Euler #1 in C++
How to change the tick of the color bar legend to black
Should a wizard buy fine inks every time he want to copy spells into his spellbook?
AppleTVs create a chatty alternate WiFi network
The Nth Gryphon Number
Printing attributes of selection in ArcPy?
Can you force honesty by using the Speak with Dead and Zone of Truth spells together?
RSA find public exponent
Why do early math courses focus on the cross sections of a cone and not on other 3D objects?
Delete free apps from library
Finds a Character based on distance and health
Planned maintenance scheduled April 23, 2019 at 23:30 UTC (7:30pm US/Eastern)
Announcing the arrival of Valued Associate #679: Cesar Manara
Unicorn Meta Zoo #1: Why another podcast?Euclidian distance - optimization and castingSpeed, distance and time calculatorFast Document Distance, C++Pairwise distance and residual calculationSplit a character string based on change of characterDistance and force calculation for a molecular dynamics simulationOpenMP SIMD Euclidean DistanceCalculating Euclidean distanceAlgorithm that parses through input of points and finds distanceReads a inFile and grades tests scores and finds max, min, and avg
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty margin-bottom:0;
$begingroup$
I need to improve the efficiency of my program and, using the profiler, have narrowed the problem down to 2 key areas, but I am having trouble coming up with ways to make the program run better.
Based on my profiler's report, it seems to be telling me that my if functions are inefficient. Whats a better way to achieve a better result?
Character* FindAttackTarget() const
float weakestHp = FLT_MAX;
Character* weakestEnemy = nullptr;
uint64_t weakestCharId = INT64_MAX;
//Only attack characters that are within attack range
auto& gameChars = m_pGame->m_gameCharacters;
for (size_t i = 0; i < gameChars.size(); ++i)
auto& c = gameChars[i];
if (Location.Dist(c.GetLocation()) <= AttackRange &&
c.HP > 0 &&
c.Team != Team)
//We want the weakest with the lowest ID number - this keeps consistent results when re-playing the same part of the game (eg. after a load game)
if (c.HP < weakestHp
return weakestEnemy;
c++ performance
New contributor
$endgroup$
add a comment |
$begingroup$
I need to improve the efficiency of my program and, using the profiler, have narrowed the problem down to 2 key areas, but I am having trouble coming up with ways to make the program run better.
Based on my profiler's report, it seems to be telling me that my if functions are inefficient. Whats a better way to achieve a better result?
Character* FindAttackTarget() const
float weakestHp = FLT_MAX;
Character* weakestEnemy = nullptr;
uint64_t weakestCharId = INT64_MAX;
//Only attack characters that are within attack range
auto& gameChars = m_pGame->m_gameCharacters;
for (size_t i = 0; i < gameChars.size(); ++i)
auto& c = gameChars[i];
if (Location.Dist(c.GetLocation()) <= AttackRange &&
c.HP > 0 &&
c.Team != Team)
//We want the weakest with the lowest ID number - this keeps consistent results when re-playing the same part of the game (eg. after a load game)
if (c.HP < weakestHp
return weakestEnemy;
c++ performance
New contributor
$endgroup$
$begingroup$
Which lines are the ones being flagged by the profiler as slow?
$endgroup$
– 1201ProgramAlarm
2 days ago
$begingroup$
Are there many characters? And are few of them in range?
$endgroup$
– Deduplicator
yesterday
$begingroup$
A Note on NamingFindAttackTarget
implies it's finding a target to attack, which it does. But the name fails to deliver any of the relevant information that the function is retrieving the one with the lowest HP or the distance factor. One might improve this with something likeFindLowestHealthTarget
.
$endgroup$
– Shelby115
yesterday
add a comment |
$begingroup$
I need to improve the efficiency of my program and, using the profiler, have narrowed the problem down to 2 key areas, but I am having trouble coming up with ways to make the program run better.
Based on my profiler's report, it seems to be telling me that my if functions are inefficient. Whats a better way to achieve a better result?
Character* FindAttackTarget() const
float weakestHp = FLT_MAX;
Character* weakestEnemy = nullptr;
uint64_t weakestCharId = INT64_MAX;
//Only attack characters that are within attack range
auto& gameChars = m_pGame->m_gameCharacters;
for (size_t i = 0; i < gameChars.size(); ++i)
auto& c = gameChars[i];
if (Location.Dist(c.GetLocation()) <= AttackRange &&
c.HP > 0 &&
c.Team != Team)
//We want the weakest with the lowest ID number - this keeps consistent results when re-playing the same part of the game (eg. after a load game)
if (c.HP < weakestHp
return weakestEnemy;
c++ performance
New contributor
$endgroup$
I need to improve the efficiency of my program and, using the profiler, have narrowed the problem down to 2 key areas, but I am having trouble coming up with ways to make the program run better.
Based on my profiler's report, it seems to be telling me that my if functions are inefficient. Whats a better way to achieve a better result?
Character* FindAttackTarget() const
float weakestHp = FLT_MAX;
Character* weakestEnemy = nullptr;
uint64_t weakestCharId = INT64_MAX;
//Only attack characters that are within attack range
auto& gameChars = m_pGame->m_gameCharacters;
for (size_t i = 0; i < gameChars.size(); ++i)
auto& c = gameChars[i];
if (Location.Dist(c.GetLocation()) <= AttackRange &&
c.HP > 0 &&
c.Team != Team)
//We want the weakest with the lowest ID number - this keeps consistent results when re-playing the same part of the game (eg. after a load game)
if (c.HP < weakestHp
return weakestEnemy;
c++ performance
c++ performance
New contributor
New contributor
New contributor
asked 2 days ago
AtazirAtazir
283
283
New contributor
New contributor
$begingroup$
Which lines are the ones being flagged by the profiler as slow?
$endgroup$
– 1201ProgramAlarm
2 days ago
$begingroup$
Are there many characters? And are few of them in range?
$endgroup$
– Deduplicator
yesterday
$begingroup$
A Note on NamingFindAttackTarget
implies it's finding a target to attack, which it does. But the name fails to deliver any of the relevant information that the function is retrieving the one with the lowest HP or the distance factor. One might improve this with something likeFindLowestHealthTarget
.
$endgroup$
– Shelby115
yesterday
add a comment |
$begingroup$
Which lines are the ones being flagged by the profiler as slow?
$endgroup$
– 1201ProgramAlarm
2 days ago
$begingroup$
Are there many characters? And are few of them in range?
$endgroup$
– Deduplicator
yesterday
$begingroup$
A Note on NamingFindAttackTarget
implies it's finding a target to attack, which it does. But the name fails to deliver any of the relevant information that the function is retrieving the one with the lowest HP or the distance factor. One might improve this with something likeFindLowestHealthTarget
.
$endgroup$
– Shelby115
yesterday
$begingroup$
Which lines are the ones being flagged by the profiler as slow?
$endgroup$
– 1201ProgramAlarm
2 days ago
$begingroup$
Which lines are the ones being flagged by the profiler as slow?
$endgroup$
– 1201ProgramAlarm
2 days ago
$begingroup$
Are there many characters? And are few of them in range?
$endgroup$
– Deduplicator
yesterday
$begingroup$
Are there many characters? And are few of them in range?
$endgroup$
– Deduplicator
yesterday
$begingroup$
A Note on Naming
FindAttackTarget
implies it's finding a target to attack, which it does. But the name fails to deliver any of the relevant information that the function is retrieving the one with the lowest HP or the distance factor. One might improve this with something like FindLowestHealthTarget
.$endgroup$
– Shelby115
yesterday
$begingroup$
A Note on Naming
FindAttackTarget
implies it's finding a target to attack, which it does. But the name fails to deliver any of the relevant information that the function is retrieving the one with the lowest HP or the distance factor. One might improve this with something like FindLowestHealthTarget
.$endgroup$
– Shelby115
yesterday
add a comment |
3 Answers
3
active
oldest
votes
$begingroup$
The tests c.HP > 0
and c.Team != Team
are probably blazingly fast tests. Location.Dist(c.GetLocation()) <= AttackRange
probably involves the square-root of the sum of the squares of the difference of coordinates in two or three dimensions. Plus, GetLocation()
may involve memory allocation and/or copying constructors. It is by far the slowest test, yet you are testing it first! Take advantage of the short-circuit logical and
operators by reordering the tests so the fastest tests are done first, so the slowest test may not even need to be executed, resulting in faster execution.
Since all conditions must pass before you update weakestEnemy
, you could even test whether or not the target passes the “weakest with lowest ID” test before checking the attack range.
Bonus: the square-root can be avoided; simply compute the square distance, and compare against the $AttackRange^2$ (computed outside of the loop) for another speed gain.
$endgroup$
$begingroup$
I believe you're on the right track but could expand and systematize your answer: for instance, @Atazir could have gameChars partitioned between enemy and the rest, then have the enemy partition sorted by strength, and last find the first enemy in range: the costly operation is performed on the least number of elements.
$endgroup$
– papagaga
2 days ago
$begingroup$
@papagaga Partitioning is a great idea; not so sure about the sorting. Perhaps it would be better if you expanded and defended these ideas in your own answer.
$endgroup$
– AJNeufeld
2 days ago
add a comment |
$begingroup$
The most obvious target for improvement is the outer loop:
Currently, you loop over all characters. If there are few characters, that's fine. If there are many, it's unconscionable.
Consider dividing your play-area into a grid, and adding a std::unordered_multimap
to find all characters in a cell. Depending on the range and your cell-size, you won't have to search too many cells, and they won't have that many characters each.
$endgroup$
add a comment |
$begingroup$
Since you tagged c++
, we should make use of C++ features, and not pretend c++
being c
with classes
.
No raw loops (see Sean Parent's talk on C++ Seasoning)
auto& c = gameChars[i];
if (Location.Dist(c.GetLocation()) <= AttackRange &&
c.HP > 0 &&
c.Team != Team)
The body of the for
loop is basically finding the minimum element in a range of Characters
, where the range is represented by Characters
, which satisfy a pre-condition. There is already a function for that in the STL called std::min_element.
In order to use std::min_element
you only have to provide a comparison functor or a pre-define a comparison function for Character
, e.g.
bool operator<(const Character& lhs, const Character& rhs)
Now considering std::min_element
and including the pre-check, we can rewrite FindAttackTarget
to:
Character* FindAttackTarget() const
auto& gameChars = m_pGame->m_gameCharacters;
auto min_it = std::min_element(gameChars.begin(), gameChars.end(), [&](auto& lhs, auto& rhs)
// also changed the order of evaluation since Location.Dist() is most likely the slowest to evaluate
if(lhs.HP > 0 && lhs.Team != Team && Location.Dist(lhs.GetLocation()) <= AttackRange)
return lhs < rhs;
return false;
));
if(min_it == gameChars.end()) // no target found
return nullptr;
return &(*min_it);
Et voila, no raw loops, only using STL algorithms. Now everyone can reason about your program without having to worry about any loop shenanigans you might have mixed up.
From here on, there can only be one bottleneck, and that is the Location.Dist()
function as @AJNeufeld already mentioned.
Addon: range-v3
With the range-v3
library, which will be included in c++20
, you can pre-filter your attackable targets like this:
bool isAttackable(const Character& c)
return c.HP > 0 && c.Team != Team && Location.Dist(c.GetLocation()) <= AttackRange;
auto attackableChars = gameChars | ranges::view::filter(isAttackable);
resulting in code for FindAttackTarget
:
Character* FindAttackTarget() const ranges::view::filter(isAttackable);
if(attackableChars.empty()) // no target found
return nullptr;
return &(*std::min_element(attackableChars.begin(), attackableChars.end()));
$endgroup$
add a comment |
Your Answer
StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");
StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);
else
createEditor();
);
function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader:
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
,
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);
);
Atazir is a new contributor. Be nice, and check out our Code of Conduct.
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f217649%2ffinds-a-character-based-on-distance-and-health%23new-answer', 'question_page');
);
Post as a guest
Required, but never shown
3 Answers
3
active
oldest
votes
3 Answers
3
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
The tests c.HP > 0
and c.Team != Team
are probably blazingly fast tests. Location.Dist(c.GetLocation()) <= AttackRange
probably involves the square-root of the sum of the squares of the difference of coordinates in two or three dimensions. Plus, GetLocation()
may involve memory allocation and/or copying constructors. It is by far the slowest test, yet you are testing it first! Take advantage of the short-circuit logical and
operators by reordering the tests so the fastest tests are done first, so the slowest test may not even need to be executed, resulting in faster execution.
Since all conditions must pass before you update weakestEnemy
, you could even test whether or not the target passes the “weakest with lowest ID” test before checking the attack range.
Bonus: the square-root can be avoided; simply compute the square distance, and compare against the $AttackRange^2$ (computed outside of the loop) for another speed gain.
$endgroup$
$begingroup$
I believe you're on the right track but could expand and systematize your answer: for instance, @Atazir could have gameChars partitioned between enemy and the rest, then have the enemy partition sorted by strength, and last find the first enemy in range: the costly operation is performed on the least number of elements.
$endgroup$
– papagaga
2 days ago
$begingroup$
@papagaga Partitioning is a great idea; not so sure about the sorting. Perhaps it would be better if you expanded and defended these ideas in your own answer.
$endgroup$
– AJNeufeld
2 days ago
add a comment |
$begingroup$
The tests c.HP > 0
and c.Team != Team
are probably blazingly fast tests. Location.Dist(c.GetLocation()) <= AttackRange
probably involves the square-root of the sum of the squares of the difference of coordinates in two or three dimensions. Plus, GetLocation()
may involve memory allocation and/or copying constructors. It is by far the slowest test, yet you are testing it first! Take advantage of the short-circuit logical and
operators by reordering the tests so the fastest tests are done first, so the slowest test may not even need to be executed, resulting in faster execution.
Since all conditions must pass before you update weakestEnemy
, you could even test whether or not the target passes the “weakest with lowest ID” test before checking the attack range.
Bonus: the square-root can be avoided; simply compute the square distance, and compare against the $AttackRange^2$ (computed outside of the loop) for another speed gain.
$endgroup$
$begingroup$
I believe you're on the right track but could expand and systematize your answer: for instance, @Atazir could have gameChars partitioned between enemy and the rest, then have the enemy partition sorted by strength, and last find the first enemy in range: the costly operation is performed on the least number of elements.
$endgroup$
– papagaga
2 days ago
$begingroup$
@papagaga Partitioning is a great idea; not so sure about the sorting. Perhaps it would be better if you expanded and defended these ideas in your own answer.
$endgroup$
– AJNeufeld
2 days ago
add a comment |
$begingroup$
The tests c.HP > 0
and c.Team != Team
are probably blazingly fast tests. Location.Dist(c.GetLocation()) <= AttackRange
probably involves the square-root of the sum of the squares of the difference of coordinates in two or three dimensions. Plus, GetLocation()
may involve memory allocation and/or copying constructors. It is by far the slowest test, yet you are testing it first! Take advantage of the short-circuit logical and
operators by reordering the tests so the fastest tests are done first, so the slowest test may not even need to be executed, resulting in faster execution.
Since all conditions must pass before you update weakestEnemy
, you could even test whether or not the target passes the “weakest with lowest ID” test before checking the attack range.
Bonus: the square-root can be avoided; simply compute the square distance, and compare against the $AttackRange^2$ (computed outside of the loop) for another speed gain.
$endgroup$
The tests c.HP > 0
and c.Team != Team
are probably blazingly fast tests. Location.Dist(c.GetLocation()) <= AttackRange
probably involves the square-root of the sum of the squares of the difference of coordinates in two or three dimensions. Plus, GetLocation()
may involve memory allocation and/or copying constructors. It is by far the slowest test, yet you are testing it first! Take advantage of the short-circuit logical and
operators by reordering the tests so the fastest tests are done first, so the slowest test may not even need to be executed, resulting in faster execution.
Since all conditions must pass before you update weakestEnemy
, you could even test whether or not the target passes the “weakest with lowest ID” test before checking the attack range.
Bonus: the square-root can be avoided; simply compute the square distance, and compare against the $AttackRange^2$ (computed outside of the loop) for another speed gain.
edited 2 days ago
answered 2 days ago
AJNeufeldAJNeufeld
7,1391723
7,1391723
$begingroup$
I believe you're on the right track but could expand and systematize your answer: for instance, @Atazir could have gameChars partitioned between enemy and the rest, then have the enemy partition sorted by strength, and last find the first enemy in range: the costly operation is performed on the least number of elements.
$endgroup$
– papagaga
2 days ago
$begingroup$
@papagaga Partitioning is a great idea; not so sure about the sorting. Perhaps it would be better if you expanded and defended these ideas in your own answer.
$endgroup$
– AJNeufeld
2 days ago
add a comment |
$begingroup$
I believe you're on the right track but could expand and systematize your answer: for instance, @Atazir could have gameChars partitioned between enemy and the rest, then have the enemy partition sorted by strength, and last find the first enemy in range: the costly operation is performed on the least number of elements.
$endgroup$
– papagaga
2 days ago
$begingroup$
@papagaga Partitioning is a great idea; not so sure about the sorting. Perhaps it would be better if you expanded and defended these ideas in your own answer.
$endgroup$
– AJNeufeld
2 days ago
$begingroup$
I believe you're on the right track but could expand and systematize your answer: for instance, @Atazir could have gameChars partitioned between enemy and the rest, then have the enemy partition sorted by strength, and last find the first enemy in range: the costly operation is performed on the least number of elements.
$endgroup$
– papagaga
2 days ago
$begingroup$
I believe you're on the right track but could expand and systematize your answer: for instance, @Atazir could have gameChars partitioned between enemy and the rest, then have the enemy partition sorted by strength, and last find the first enemy in range: the costly operation is performed on the least number of elements.
$endgroup$
– papagaga
2 days ago
$begingroup$
@papagaga Partitioning is a great idea; not so sure about the sorting. Perhaps it would be better if you expanded and defended these ideas in your own answer.
$endgroup$
– AJNeufeld
2 days ago
$begingroup$
@papagaga Partitioning is a great idea; not so sure about the sorting. Perhaps it would be better if you expanded and defended these ideas in your own answer.
$endgroup$
– AJNeufeld
2 days ago
add a comment |
$begingroup$
The most obvious target for improvement is the outer loop:
Currently, you loop over all characters. If there are few characters, that's fine. If there are many, it's unconscionable.
Consider dividing your play-area into a grid, and adding a std::unordered_multimap
to find all characters in a cell. Depending on the range and your cell-size, you won't have to search too many cells, and they won't have that many characters each.
$endgroup$
add a comment |
$begingroup$
The most obvious target for improvement is the outer loop:
Currently, you loop over all characters. If there are few characters, that's fine. If there are many, it's unconscionable.
Consider dividing your play-area into a grid, and adding a std::unordered_multimap
to find all characters in a cell. Depending on the range and your cell-size, you won't have to search too many cells, and they won't have that many characters each.
$endgroup$
add a comment |
$begingroup$
The most obvious target for improvement is the outer loop:
Currently, you loop over all characters. If there are few characters, that's fine. If there are many, it's unconscionable.
Consider dividing your play-area into a grid, and adding a std::unordered_multimap
to find all characters in a cell. Depending on the range and your cell-size, you won't have to search too many cells, and they won't have that many characters each.
$endgroup$
The most obvious target for improvement is the outer loop:
Currently, you loop over all characters. If there are few characters, that's fine. If there are many, it's unconscionable.
Consider dividing your play-area into a grid, and adding a std::unordered_multimap
to find all characters in a cell. Depending on the range and your cell-size, you won't have to search too many cells, and they won't have that many characters each.
answered yesterday
DeduplicatorDeduplicator
11.9k1950
11.9k1950
add a comment |
add a comment |
$begingroup$
Since you tagged c++
, we should make use of C++ features, and not pretend c++
being c
with classes
.
No raw loops (see Sean Parent's talk on C++ Seasoning)
auto& c = gameChars[i];
if (Location.Dist(c.GetLocation()) <= AttackRange &&
c.HP > 0 &&
c.Team != Team)
The body of the for
loop is basically finding the minimum element in a range of Characters
, where the range is represented by Characters
, which satisfy a pre-condition. There is already a function for that in the STL called std::min_element.
In order to use std::min_element
you only have to provide a comparison functor or a pre-define a comparison function for Character
, e.g.
bool operator<(const Character& lhs, const Character& rhs)
Now considering std::min_element
and including the pre-check, we can rewrite FindAttackTarget
to:
Character* FindAttackTarget() const
auto& gameChars = m_pGame->m_gameCharacters;
auto min_it = std::min_element(gameChars.begin(), gameChars.end(), [&](auto& lhs, auto& rhs)
// also changed the order of evaluation since Location.Dist() is most likely the slowest to evaluate
if(lhs.HP > 0 && lhs.Team != Team && Location.Dist(lhs.GetLocation()) <= AttackRange)
return lhs < rhs;
return false;
));
if(min_it == gameChars.end()) // no target found
return nullptr;
return &(*min_it);
Et voila, no raw loops, only using STL algorithms. Now everyone can reason about your program without having to worry about any loop shenanigans you might have mixed up.
From here on, there can only be one bottleneck, and that is the Location.Dist()
function as @AJNeufeld already mentioned.
Addon: range-v3
With the range-v3
library, which will be included in c++20
, you can pre-filter your attackable targets like this:
bool isAttackable(const Character& c)
return c.HP > 0 && c.Team != Team && Location.Dist(c.GetLocation()) <= AttackRange;
auto attackableChars = gameChars | ranges::view::filter(isAttackable);
resulting in code for FindAttackTarget
:
Character* FindAttackTarget() const ranges::view::filter(isAttackable);
if(attackableChars.empty()) // no target found
return nullptr;
return &(*std::min_element(attackableChars.begin(), attackableChars.end()));
$endgroup$
add a comment |
$begingroup$
Since you tagged c++
, we should make use of C++ features, and not pretend c++
being c
with classes
.
No raw loops (see Sean Parent's talk on C++ Seasoning)
auto& c = gameChars[i];
if (Location.Dist(c.GetLocation()) <= AttackRange &&
c.HP > 0 &&
c.Team != Team)
The body of the for
loop is basically finding the minimum element in a range of Characters
, where the range is represented by Characters
, which satisfy a pre-condition. There is already a function for that in the STL called std::min_element.
In order to use std::min_element
you only have to provide a comparison functor or a pre-define a comparison function for Character
, e.g.
bool operator<(const Character& lhs, const Character& rhs)
Now considering std::min_element
and including the pre-check, we can rewrite FindAttackTarget
to:
Character* FindAttackTarget() const
auto& gameChars = m_pGame->m_gameCharacters;
auto min_it = std::min_element(gameChars.begin(), gameChars.end(), [&](auto& lhs, auto& rhs)
// also changed the order of evaluation since Location.Dist() is most likely the slowest to evaluate
if(lhs.HP > 0 && lhs.Team != Team && Location.Dist(lhs.GetLocation()) <= AttackRange)
return lhs < rhs;
return false;
));
if(min_it == gameChars.end()) // no target found
return nullptr;
return &(*min_it);
Et voila, no raw loops, only using STL algorithms. Now everyone can reason about your program without having to worry about any loop shenanigans you might have mixed up.
From here on, there can only be one bottleneck, and that is the Location.Dist()
function as @AJNeufeld already mentioned.
Addon: range-v3
With the range-v3
library, which will be included in c++20
, you can pre-filter your attackable targets like this:
bool isAttackable(const Character& c)
return c.HP > 0 && c.Team != Team && Location.Dist(c.GetLocation()) <= AttackRange;
auto attackableChars = gameChars | ranges::view::filter(isAttackable);
resulting in code for FindAttackTarget
:
Character* FindAttackTarget() const ranges::view::filter(isAttackable);
if(attackableChars.empty()) // no target found
return nullptr;
return &(*std::min_element(attackableChars.begin(), attackableChars.end()));
$endgroup$
add a comment |
$begingroup$
Since you tagged c++
, we should make use of C++ features, and not pretend c++
being c
with classes
.
No raw loops (see Sean Parent's talk on C++ Seasoning)
auto& c = gameChars[i];
if (Location.Dist(c.GetLocation()) <= AttackRange &&
c.HP > 0 &&
c.Team != Team)
The body of the for
loop is basically finding the minimum element in a range of Characters
, where the range is represented by Characters
, which satisfy a pre-condition. There is already a function for that in the STL called std::min_element.
In order to use std::min_element
you only have to provide a comparison functor or a pre-define a comparison function for Character
, e.g.
bool operator<(const Character& lhs, const Character& rhs)
Now considering std::min_element
and including the pre-check, we can rewrite FindAttackTarget
to:
Character* FindAttackTarget() const
auto& gameChars = m_pGame->m_gameCharacters;
auto min_it = std::min_element(gameChars.begin(), gameChars.end(), [&](auto& lhs, auto& rhs)
// also changed the order of evaluation since Location.Dist() is most likely the slowest to evaluate
if(lhs.HP > 0 && lhs.Team != Team && Location.Dist(lhs.GetLocation()) <= AttackRange)
return lhs < rhs;
return false;
));
if(min_it == gameChars.end()) // no target found
return nullptr;
return &(*min_it);
Et voila, no raw loops, only using STL algorithms. Now everyone can reason about your program without having to worry about any loop shenanigans you might have mixed up.
From here on, there can only be one bottleneck, and that is the Location.Dist()
function as @AJNeufeld already mentioned.
Addon: range-v3
With the range-v3
library, which will be included in c++20
, you can pre-filter your attackable targets like this:
bool isAttackable(const Character& c)
return c.HP > 0 && c.Team != Team && Location.Dist(c.GetLocation()) <= AttackRange;
auto attackableChars = gameChars | ranges::view::filter(isAttackable);
resulting in code for FindAttackTarget
:
Character* FindAttackTarget() const ranges::view::filter(isAttackable);
if(attackableChars.empty()) // no target found
return nullptr;
return &(*std::min_element(attackableChars.begin(), attackableChars.end()));
$endgroup$
Since you tagged c++
, we should make use of C++ features, and not pretend c++
being c
with classes
.
No raw loops (see Sean Parent's talk on C++ Seasoning)
auto& c = gameChars[i];
if (Location.Dist(c.GetLocation()) <= AttackRange &&
c.HP > 0 &&
c.Team != Team)
The body of the for
loop is basically finding the minimum element in a range of Characters
, where the range is represented by Characters
, which satisfy a pre-condition. There is already a function for that in the STL called std::min_element.
In order to use std::min_element
you only have to provide a comparison functor or a pre-define a comparison function for Character
, e.g.
bool operator<(const Character& lhs, const Character& rhs)
Now considering std::min_element
and including the pre-check, we can rewrite FindAttackTarget
to:
Character* FindAttackTarget() const
auto& gameChars = m_pGame->m_gameCharacters;
auto min_it = std::min_element(gameChars.begin(), gameChars.end(), [&](auto& lhs, auto& rhs)
// also changed the order of evaluation since Location.Dist() is most likely the slowest to evaluate
if(lhs.HP > 0 && lhs.Team != Team && Location.Dist(lhs.GetLocation()) <= AttackRange)
return lhs < rhs;
return false;
));
if(min_it == gameChars.end()) // no target found
return nullptr;
return &(*min_it);
Et voila, no raw loops, only using STL algorithms. Now everyone can reason about your program without having to worry about any loop shenanigans you might have mixed up.
From here on, there can only be one bottleneck, and that is the Location.Dist()
function as @AJNeufeld already mentioned.
Addon: range-v3
With the range-v3
library, which will be included in c++20
, you can pre-filter your attackable targets like this:
bool isAttackable(const Character& c)
return c.HP > 0 && c.Team != Team && Location.Dist(c.GetLocation()) <= AttackRange;
auto attackableChars = gameChars | ranges::view::filter(isAttackable);
resulting in code for FindAttackTarget
:
Character* FindAttackTarget() const ranges::view::filter(isAttackable);
if(attackableChars.empty()) // no target found
return nullptr;
return &(*std::min_element(attackableChars.begin(), attackableChars.end()));
edited yesterday
answered yesterday
kanstarkanstar
1745
1745
add a comment |
add a comment |
Atazir is a new contributor. Be nice, and check out our Code of Conduct.
Atazir is a new contributor. Be nice, and check out our Code of Conduct.
Atazir is a new contributor. Be nice, and check out our Code of Conduct.
Atazir is a new contributor. Be nice, and check out our Code of Conduct.
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f217649%2ffinds-a-character-based-on-distance-and-health%23new-answer', 'question_page');
);
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
$begingroup$
Which lines are the ones being flagged by the profiler as slow?
$endgroup$
– 1201ProgramAlarm
2 days ago
$begingroup$
Are there many characters? And are few of them in range?
$endgroup$
– Deduplicator
yesterday
$begingroup$
A Note on Naming
FindAttackTarget
implies it's finding a target to attack, which it does. But the name fails to deliver any of the relevant information that the function is retrieving the one with the lowest HP or the distance factor. One might improve this with something likeFindLowestHealthTarget
.$endgroup$
– Shelby115
yesterday