Array curve #1724

jere8184 · 2024-12-03T20:31:39Z

heinezen · 2024-12-09T12:57:20Z

libopenage/curve/array.h

+	// std::array<Keyframe<T>, size> get_all(const time::time_t& t, const size_t hint) const;
+
+
+	size_t size() const;


You can make this method constexpr since it returns Size which is known at compile time.

Maybe you can even make it consteval but I think that doesn't work on class methods.

heinezen · 2024-12-09T21:22:03Z

libopenage/curve/CMakeLists.txt

@@ -12,6 +12,7 @@ add_sources(libopenage
 	queue.cpp
 	queue_filter_iterator.cpp
 	segmented.cpp
+	array.cpp


the list of files should be sorted alphabetically

heinezen · 2024-12-09T21:23:18Z

libopenage/curve/array.cpp

@@ -0,0 +1,10 @@
+// Copyright 2017-2024 the openage authors. See copying.md for legal info.


The copyright year format is {created}-{last changed}. Since you created the file in 2024, it should be 2024-2024

heinezen · 2024-12-09T21:23:27Z

libopenage/curve/array.h

@@ -0,0 +1,159 @@
+// Copyright 2017-2024 the openage authors. See copying.md for legal info.


Suggested change

// Copyright 2017-2024 the openage authors. See copying.md for legal info.

// Copyright 2024-2024 the openage authors. See copying.md for legal info.

heinezen · 2024-12-09T21:28:34Z

libopenage/curve/array.h

+
+	size_t size() const;
+
+	const Keyframe<T> &frame(const time::time_t &t, const size_t index) const;


You should never expose Keyframe references directly. The reasoning here is that a Keyframe may be dumped to file or deleted for optimization purposes over time. Therefore, you should return a pair instead, like in

openage/libopenage/curve/base_curve.h

Line 64 in 2b902a9

virtual std::pair<time::time_t, const T> frame(const time::time_t &time) const;

heinezen · 2024-12-09T21:28:43Z

libopenage/curve/array.h

+	const Keyframe<T> &frame(const time::time_t &t, const size_t index) const;
+
+
+	const Keyframe<T> &next_frame(const time::time_t &t, const size_t index) const;


heinezen · 2024-12-09T21:33:40Z

libopenage/curve/array.h

+
+private:
+	container_t container;
+	mutable size_t last_hit_index = 0;


What is this member supposed to do?

it acts as a hint, for KeyframeContainer operations.

But what is the hit index intended to be used for? From what I can see, it's an index into a KeyframeContainer, but there are Size KeyframeContainers in the array.

array of hints?

Hmm okay I think I understand now what this is used for. An array of hints (one for each container) would work better, yes.

heinezen · 2024-12-09T21:37:54Z

libopenage/curve/array.h

+	for (int i = 0; i < Size; i++) {
+		this->container[i].sync(other, start);
+	}


If you are iterating through all element, you should use a range-based for-loop.

There was a mistake, here
I forgot to Index other as well.

jere8184 · 2024-12-11T17:43:55Z

is there any chance array curve should be an array of base curves instead of an array of key frame containers?

heinezen · 2024-12-12T18:19:44Z

is there any chance array curve should be an array of base curves instead of an array of key frame containers?

The base curves would have a bunch of duplicate info that we would want to avoid, e.g. they would all have shared pointer to the event loop, their own IDs, and ID strings. Basically, the curve array container should only have this information once.

heinezen · 2024-12-12T18:21:40Z

libopenage/curve/array.h

+namespace curve {
+
+template <typename T, size_t Size>
+class Array {


the class should inherit from event::EventEntity like all curve types.

heinezen · 2024-12-12T21:37:17Z

would making the base curves store references to these shared fields solve this issue?

Then the IDs wouldn't be unique which would be a problem for the event system. Since all curves are event entities, duplicating the information would either lead to all contained curves being updated or worse, only one curve being tracked by the event loop. It would also be a waste of space, since we have to store a bunch of redundant data which is what we want to avoid with a dedicated Array class.

Also, BaseCurve is an interface and should not be instantiated. It doesn't make sense to use it as a container because KeyframeContainer mostly fulfills that jopb already.

heinezen marked this pull request as draft December 3, 2024 21:38

jere8184 force-pushed the array_curve branch from a28ebc2 to fb2a888 Compare December 9, 2024 01:05

heinezen reviewed Dec 10, 2024

View reviewed changes

jere8184 force-pushed the array_curve branch from 3ea0f4a to 4c2b2f3 Compare December 11, 2024 03:29

jere8184 force-pushed the array_curve branch from 4c2b2f3 to 0b01820 Compare December 11, 2024 17:58

heinezen reviewed Dec 12, 2024

View reviewed changes

jere8184 force-pushed the array_curve branch from 0b01820 to 1d653cd Compare December 13, 2024 16:17

jere8184 marked this pull request as ready for review December 15, 2024 22:31

jere8184 force-pushed the array_curve branch 3 times, most recently from 79e5549 to f68ec30 Compare December 21, 2024 14:24

impliment curve::array

0e637b0

jere8184 force-pushed the array_curve branch from f68ec30 to 0e637b0 Compare December 21, 2024 18:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Array curve #1724

Array curve #1724

jere8184 commented Dec 3, 2024

heinezen Dec 9, 2024

heinezen Dec 9, 2024

heinezen Dec 9, 2024

heinezen Dec 9, 2024

heinezen Dec 9, 2024

heinezen Dec 9, 2024

heinezen Dec 9, 2024

heinezen Dec 9, 2024

jere8184 Dec 11, 2024

heinezen Dec 12, 2024

jere8184 Dec 12, 2024

heinezen Dec 12, 2024

heinezen Dec 9, 2024

jere8184 Dec 16, 2024

jere8184 commented Dec 11, 2024

heinezen commented Dec 12, 2024

heinezen Dec 12, 2024

heinezen commented Dec 12, 2024

		// std::array<Keyframe<T>, size> get_all(const time::time_t& t, const size_t hint) const;


		size_t size() const;

		@@ -0,0 +1,10 @@
		// Copyright 2017-2024 the openage authors. See copying.md for legal info.

		@@ -0,0 +1,159 @@
		// Copyright 2017-2024 the openage authors. See copying.md for legal info.

	// Copyright 2017-2024 the openage authors. See copying.md for legal info.
	// Copyright 2024-2024 the openage authors. See copying.md for legal info.


		size_t size() const;

		const Keyframe<T> &frame(const time::time_t &t, const size_t index) const;

		const Keyframe<T> &frame(const time::time_t &t, const size_t index) const;


		const Keyframe<T> &next_frame(const time::time_t &t, const size_t index) const;

Array curve #1724

Are you sure you want to change the base?

Array curve #1724

Conversation

jere8184 commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jere8184 commented Dec 11, 2024

heinezen commented Dec 12, 2024

Choose a reason for hiding this comment

heinezen commented Dec 12, 2024