Pathfinder history #1748
Pathfinder history #1748
Conversation
Co-authored-by: jere8184 <[email protected]>
jere8184
force-pushed
the
pathfinder_history
branch
from
2a3d646 to
df2f0ab
Compare
| /** | ||
| * Array curve recording cell cost history, | ||
| */ | ||
| curve::Array<cost_t, CHUNK_SIZE> cell_cost_history; |
There was a problem hiding this comment.
How should cell_cost_history be exposed? A simple getter that returns a constant ref to the curve?
There was a problem hiding this comment.
Yes that would be enough I think.
jere8184
force-pushed
the
pathfinder_history
branch
from
df2f0ab to
8d216b9
Compare
| void set_insert_range(const time::time_t &t, auto begin_it, auto end_it) { | ||
| ENSURE(std::distance(begin_it, end_it) <= Size, | ||
| "trying to insert more values than there are postions: max allowed = " << Size); | ||
| index_t i = 0; | ||
| std::for_each(begin_it, end_it, [&](const T &val) { this->set_insert(t, i++, val); }); | ||
| } |
There was a problem hiding this comment.
Nice idea. Would it also make sense to make index configurable? Then we could even replace slices of the array.
| @@ -105,6 +106,14 @@ class Array : event::EventEntity { | |||
| */ | |||
| std::pair<time::time_t, T> next_frame(const time::time_t &t, const index_t index) const; | |||
|
|
|||
|
|
|||
| void set_insert_range(const time::time_t &t, auto begin_it, auto end_it) { | |||
There was a problem hiding this comment.
Also, would be nice if the method gets documented :)
| } | ||
|
|
||
| bool CostField::stamp(size_t idx, cost_t cost, const time::time_t &stamped_at) { | ||
| if (this->cost_stamps[idx].has_value()) return false; |
There was a problem hiding this comment.
Please always make if clauses multi-line for consistent code style.
| if (this->cost_stamps[idx].has_value()) return false; | |
| if (this->cost_stamps[idx].has_value()) { | |
| return false; | |
| } |
| } | ||
|
|
||
| bool CostField::unstamp(size_t idx, const time::time_t &unstamped_at) { | ||
| if (!this->cost_stamps[idx].has_value() || unstamped_at < this->cost_stamps[idx]->stamp_time) return false; |
There was a problem hiding this comment.
Same here and not/and/or should be used as operators for better readability.
| if (!this->cost_stamps[idx].has_value() || unstamped_at < this->cost_stamps[idx]->stamp_time) return false; | |
| if (not this->cost_stamps[idx].has_value() or unstamped_at < this->cost_stamps[idx]->stamp_time) { | |
| return false; | |
| } |
| @@ -14,6 +16,8 @@ namespace coord { | |||
| struct tile_delta; | |||
| } // namespace coord | |||
|
|
|||
| const unsigned int CHUNK_SIZE = 100; | |||
There was a problem hiding this comment.
The value should probably not be hardcoded because then the cost field size is not configurable anymore. I think we either need to make CostField a templated class or wait for the implementation of curve::Vector in #1677 . Both approaches would have pros and cons.
| /** | ||
| * Array curve recording cell cost history, | ||
| */ | ||
| curve::Array<cost_t, CHUNK_SIZE> cell_cost_history; |
There was a problem hiding this comment.
Yes that would be enough I think.
| /** | ||
| * Cost stamp for a given cost field cell. | ||
| */ |
There was a problem hiding this comment.
A small explanation of stamping for people who don't know what that is would be nice here. I added a suggesion below.
| /** | |
| * Cost stamp for a given cost field cell. | |
| */ | |
| /** | |
| * Cost stamp for a cell on a cost field. | |
| * | |
| * Stamps are used when cell costs are temporarily overwritten, e.g. when placing a game object | |
| * as an obstacle over terrain. Once the obstacle is removed, the cell can be reset to its original | |
| * value recorded by the stamp. | |
| */ |
heinezen
added
improvement
fixes: #1680