Array curve

[jere8184]

Closes #1678

[jere8184]

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

[heinezen]

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]

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]

Some additional remarks:

1. You should really try to comment your code more (i.e. docstrings and member descriptions). It gets much harder to review your code if I also have to find out the purpose of your methods/members.
2. You should also document the new array curve type in the docs: https://github.com/SFTtech/openage/blob/master/doc/code/curves.md
3. The code looks very good so far, although I might have a few enhancement ideas after integrating the current review comments ^^

[jere8184]

• You should really try to comment your code more (i.e. docstrings and member descriptions). It gets much harder to review your code if I also have to find out the purpose of your methods/members.
• You should also document the new array curve type in the docs: https://github.com/SFTtech/openage/blob/master/doc/code/curves.md
• The code looks very good so far, although I might have a few enhancement ideas after integrating the current review comments ^^

alright, currently implementing the changes, really appreciate all the feedback.

[heinezen]

I think I found everything I wanted to comment on :D

[heinezen]

You are missing a constructor for types T that cannot/shouldn't be default-initialized. For these types, you have to pass an initial array of values std::array<T, Size> to the curve that the KeyframeContainer is initialized with.

[heinezen]

// does not create a docstring as it is used for code comments. /// creates a docstring.

Although it's much better to keep the comments consistens and always use /** ... */.

Suggested change

	// Get the amount of KeyframeContainers in array curve
	/**
	* Get the amount of KeyframeContainers in array curve.
	*/

[heinezen]

This method should not be public because KeyframeContainers should not be exposed by the curve.

[heinezen]

If you want a TODO for something that is necessary before merging, you should prefix it with ASDF. This will create a CI failure in the sanity check stage.

Suggested change

	// remember to update docs
	// ASDF: remember to update docs

[heinezen]

Is there a reason why this class doesn't implement CurveIterator and uses a different strategy for iterating?

[heinezen]

1. This method doesn't return a keyframe, it returns T
2. If it only returns T, you should use at() instead of frame() to retrieve the value.