Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Array curve #1724

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Array curve #1724

wants to merge 4 commits into from

Conversation

jere8184
Copy link
Contributor

@jere8184 jere8184 commented Dec 3, 2024

Closes #1678

@heinezen heinezen marked this pull request as draft December 3, 2024 21:38
libopenage/curve/array.h Outdated Show resolved Hide resolved
libopenage/curve/CMakeLists.txt Outdated Show resolved Hide resolved
libopenage/curve/array.cpp Outdated Show resolved Hide resolved
libopenage/curve/array.h Outdated Show resolved Hide resolved
libopenage/curve/array.h Outdated Show resolved Hide resolved
libopenage/curve/array.h Outdated Show resolved Hide resolved
libopenage/curve/array.h Outdated Show resolved Hide resolved
libopenage/curve/array.h Outdated Show resolved Hide resolved
@jere8184
Copy link
Contributor Author

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

@heinezen
Copy link
Member

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.

libopenage/curve/array.h Outdated Show resolved Hide resolved
@heinezen
Copy link
Member

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.

@jere8184 jere8184 marked this pull request as ready for review December 15, 2024 22:31
@jere8184 jere8184 force-pushed the array_curve branch 3 times, most recently from 79e5549 to f68ec30 Compare December 21, 2024 14:24
libopenage/curve/array.h Outdated Show resolved Hide resolved
libopenage/curve/array.h Outdated Show resolved Hide resolved
libopenage/curve/array.h Outdated Show resolved Hide resolved
libopenage/curve/array.h Show resolved Hide resolved
libopenage/curve/array.h Outdated Show resolved Hide resolved
libopenage/curve/tests/curve_types.cpp Outdated Show resolved Hide resolved
libopenage/curve/tests/curve_types.cpp Outdated Show resolved Hide resolved
libopenage/curve/tests/curve_types.cpp Outdated Show resolved Hide resolved
libopenage/curve/tests/curve_types.cpp Outdated Show resolved Hide resolved
libopenage/curve/tests/curve_types.cpp Outdated Show resolved Hide resolved
@heinezen
Copy link
Member

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
Copy link
Contributor Author

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

@jere8184 jere8184 force-pushed the array_curve branch 2 times, most recently from bffc8a5 to 22d7740 Compare December 26, 2024 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array Curve container
2 participants