Pathfinder history #1748

jere8184 · 2025-01-26T20:55:15Z

jere8184 · 2025-01-26T21:19:55Z

libopenage/pathfinding/cost_field.h

+	/**
+	 * Array curve recording cell cost history,
+	 */
+	curve::Array<cost_t, CHUNK_SIZE> cell_cost_history;


How should cell_cost_history be exposed? A simple getter that returns a constant ref to the curve?

Yes that would be enough I think.

heinezen

Sorry for the long wait, I was very busy with work 😅 Now that I have some time, I added a few suggestions to the code.

heinezen · 2025-04-08T22:56:23Z

libopenage/curve/container/array.h

+	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); });
+	}


Nice idea. Would it also make sense to make index configurable? Then we could even replace slices of the array.

heinezen · 2025-04-08T22:56:42Z

libopenage/curve/container/array.h

@@ -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) {


Also, would be nice if the method gets documented :)

heinezen · 2025-04-08T23:00:54Z

libopenage/pathfinding/cost_field.cpp

+}
+
+bool CostField::stamp(size_t idx, cost_t cost, const time::time_t &stamped_at) {
+	if (this->cost_stamps[idx].has_value()) return false;


Please always make if clauses multi-line for consistent code style.

Suggested change

if (this->cost_stamps[idx].has_value()) return false;

if (this->cost_stamps[idx].has_value()) {

return false;

}

heinezen · 2025-04-08T23:02:49Z

libopenage/pathfinding/cost_field.cpp

+}
+
+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;


Same here and not/and/or should be used as operators for better readability.

Suggested change

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;

}

heinezen · 2025-04-08T23:08:46Z

libopenage/pathfinding/cost_field.h

@@ -14,6 +16,8 @@ namespace coord {
 struct tile_delta;
 } // namespace coord

+const unsigned int CHUNK_SIZE = 100;


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.

Which way do you think we should go?

Let's do templates first then

Alot of classes contain a cost_field, plus cost_field itself is constructed with a size_t, should we also templatise the encapsulating classes and get rid of the constructor parameter?

yeah I think that's a good idea. I don't remember the size ever changing and see no reason why it would change.

sounds good, didnt mean to request another review btw

sry I couldn't help but take a look 😅

heinezen · 2025-04-08T23:09:09Z

libopenage/pathfinding/cost_field.h

+	/**
+	 * Array curve recording cell cost history,
+	 */
+	curve::Array<cost_t, CHUNK_SIZE> cell_cost_history;


Yes that would be enough I think.

heinezen · 2025-04-08T23:16:20Z

libopenage/pathfinding/types.h

+/**
+ * Cost stamp for a given cost field cell.
+ */


A small explanation of stamping for people who don't know what that is would be nice here. I added a suggesion below.

Suggested change

/**

* 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.

*/

Co-authored-by: jere8184 <jeremiahmorgan.bham@outlook.com>

heinezen · 2025-05-25T22:26:53Z

libopenage/curve/container/array.h

+	Array(const std::shared_ptr<event::EventLoop> &loop = nullptr,
+	      size_t id = 0,


This is not a good idea, since it will lead to errors when the curve is used for events without initializing the loop.

heinezen · 2025-05-25T22:35:44Z

libopenage/pathfinding/cost_field.h

@@ -135,7 +168,111 @@ class CostField {
 	 * Cost field values.
 	 */
 	std::vector<cost_t> cells;


This can now also be an array of size N.

heinezen · 2025-05-25T22:36:05Z

libopenage/pathfinding/cost_field.h

+	/**
+	 * Cost stamp vector.
+	 */
+	std::vector<std::optional<cost_stamp_t>> cost_stamps;


heinezen · 2025-05-25T22:37:15Z

libopenage/pathfinding/cost_field.h

 };

+template <size_t N>
+CostField<N>::CostField(size_t size) :
+	size{size},


size is no longer necessary due to templating

heinezen · 2025-05-25T22:40:06Z

libopenage/pathfinding/cost_field.h

@@ -19,6 +28,8 @@ namespace path {
 /**
 * Cost field in the flow-field pathfinding algorithm.
 */
+
+template <size_t N>


You should actually use N for more than curve::Array. Otherwise you have a mismatch, e.g. of the size of cells and cell_cost_history.

also N should be side length like size passed to the constructor. If you don't do this, then this allows non-square fields which would break pathfinding.

Wouldn't it make more sense to have a curve::Vector here than an Array? size is not a compile time constant in Grid/Sector.

@WillemKauf That's what my comment in #1748 (comment) is related to ^^

heinezen · 2025-05-25T22:46:41Z

libopenage/pathfinding/demo/demo_0.cpp

@@ -32,7 +32,7 @@ void path_demo_0(const util::Path &path) {
 	auto field_length = 10;

 	// Cost field with some obstacles
-	auto cost_field = std::make_shared<CostField>(field_length);
+	auto cost_field = std::make_shared<CostField<100>>(field_length);


You should define this size as a constant somewhere in the header file.

heinezen · 2025-05-25T22:51:22Z

libopenage/pathfinding/demo/demo_0.cpp

@@ -203,7 +203,7 @@ void RenderManager0::run() {
 	this->window->close();
 }

-void RenderManager0::show_cost_field(const std::shared_ptr<path::CostField> &field) {
+void RenderManager0::show_cost_field(const std::shared_ptr<path::CostField<100>> &field) {


use constant from header file here

heinezen · 2025-05-25T22:51:27Z

libopenage/pathfinding/demo/demo_0.cpp

@@ -551,7 +551,7 @@ void RenderManager0::init_passes() {
 		renderer->get_display_target());
 }

-renderer::resources::MeshData RenderManager0::get_cost_field_mesh(const std::shared_ptr<CostField> &field,
+renderer::resources::MeshData RenderManager0::get_cost_field_mesh(const std::shared_ptr<CostField<100>> &field,


use constant from header file here

heinezen · 2025-05-25T22:54:13Z

libopenage/pathfinding/integration_field.cpp

@@ -37,7 +37,7 @@ const integrated_t &IntegrationField::get_cell(size_t idx) const {
 	return this->cells.at(idx);
 }

-std::vector<size_t> IntegrationField::integrate_los(const std::shared_ptr<CostField> &cost_field,
+std::vector<size_t> IntegrationField::integrate_los(const std::shared_ptr<CostField<100>> &cost_field,


Doing it like this will result in IntegrationField only being compatible with cost fields of size 100. This would not only make the pathfinder less flexible, it also defeats the point of templating CostField. If you template the size of cost field, you must template the size of IntegrationField (and FlowField, Sector, etc.) as well!

jere8184 force-pushed the pathfinder_history branch from 2a3d646 to df2f0ab Compare January 26, 2025 20:57

jere8184 commented Jan 26, 2025

View reviewed changes

jere8184 force-pushed the pathfinder_history branch from df2f0ab to 8d216b9 Compare January 27, 2025 00:38

heinezen requested changes Apr 8, 2025

View reviewed changes

heinezen added the area: simulation Involved in the game mechanics and simulation label Apr 8, 2025

github-project-automation bot added this to openage game simulation Apr 8, 2025

github-project-automation bot moved this to 📋 Backlog in openage game simulation Apr 8, 2025

heinezen added improvement Enhancement of an existing component lang: c++ Done in C++ code labels Apr 8, 2025

heinezen moved this from 📋 Backlog to 👀 In review in openage game simulation Apr 8, 2025

dmwever and others added 10 commits May 25, 2025 18:35

Add cost stamps

f453ca0

Update types.h

ffeeefb

sanity checks

79beb3f

use vector of optional cost_stamp_t's

c637f4a

fix issues with optional

d68a40d

Update cost_field.h

50d7272

Update libopenage/pathfinding/cost_field.cpp

4ae4de0

Co-authored-by: jere8184 <jeremiahmorgan.bham@outlook.com>

Curve: default constructor args and pass values by ref

96024ff

Pathfinding: Record history of cost changes in pathfinder

4615e8d

Templatise

b390440

jere8184 force-pushed the pathfinder_history branch from 63c919d to b390440 Compare May 25, 2025 18:08

jere8184 requested a review from heinezen May 25, 2025 19:22

heinezen requested changes May 25, 2025

View reviewed changes

github-project-automation bot moved this from 👀 In review to 🔖 TODO in openage game simulation May 25, 2025

-/**
- * 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.
+ */

		Array(const std::shared_ptr<event::EventLoop> &loop = nullptr,
		size_t id = 0,

Uh oh!

Pathfinder history #1748

Are you sure you want to change the base?

Pathfinder history #1748

Uh oh!

Conversation

jere8184 commented Jan 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

heinezen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jere8184 May 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jere8184 commented Jan 26, 2025 •

edited

Loading

jere8184 May 25, 2025 •

edited

Loading