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 container #1678

Open
heinezen opened this issue Aug 25, 2024 · 18 comments · May be fixed by #1724
Open

Array Curve container #1678

heinezen opened this issue Aug 25, 2024 · 18 comments · May be fixed by #1724
Labels
area: util Utilitis and data structures lang: c++ Done in C++ code nice new thing ☺ A new feature that was not there before

Comments

@heinezen
Copy link
Member

heinezen commented Aug 25, 2024

We should add a curve::Array type to our curve container classes to record changes to a fixed size sequence over time. In comparison to #1677 we can probably optimize a lot more since the size doesn't change over time (i.e. no inserts or erasures).

The new array type should implement these methods:

Read

Method Description
get(t, i) Get value of index i at time t
get(t) Get array at time t
size() Get size of the array
frame(t, i) Get the previous keyframe (time and value) of index i before or at t
next_frame(t, i) Get the next keyframe (time and value) of index i after t

There should also be an iterator type to iterate over the array at time t.

Modify

Method Description
set_insert(t, i, value) Insert a new keyframe value at time t at index i
set_last(t, i, value) Insert a new keyframe value at time t; delete all keyframes after time t at index i
set_replace(t, i, value) Insert a new keyframe value at time t at index i; remove all other keyframes with time t at index i

Copy

Method Description
sync(Curve, t) Replace all keyframes from self after time t with keyframes from source Curve after time t
@heinezen heinezen added nice new thing ☺ A new feature that was not there before lang: c++ Done in C++ code area: util Utilitis and data structures labels Aug 25, 2024
@jere8184
Copy link
Contributor

jere8184 commented Dec 2, 2024

@heinezen can I take this

@heinezen
Copy link
Member Author

heinezen commented Dec 2, 2024

Sure, I think there's currently nothing blocking this feature.

@jere8184
Copy link
Contributor

jere8184 commented Dec 3, 2024

So will this be an array of KeyframeContainers , essentially?

@heinezen
Copy link
Member Author

heinezen commented Dec 3, 2024

Yeah that's basically it.

@jere8184
Copy link
Contributor

jere8184 commented Dec 3, 2024

Should I use something other than KeyframeContainers, queue and map don't seem to use it.

@heinezen
Copy link
Member Author

heinezen commented Dec 3, 2024

For the array curve, KeyframeContainers are fine because the size of the array is fixed, so all elements exist with a value over the duration of the array's existance. For the other containers, elements are not guaranteed to always be persistent. They can get erased and later added again, which has to be accounted for (that's why there's this dead/alive logic for their keyframes).

@jere8184
Copy link
Contributor

jere8184 commented Dec 3, 2024

So for the iterator, do we iterate over keyframes with the closest time less than or equal to t?

@jere8184 jere8184 linked a pull request Dec 3, 2024 that will close this issue
@heinezen
Copy link
Member Author

heinezen commented Dec 3, 2024

Yes, when iterating over the array at time t, it should iterate over the elements that are valid at time t

So the result should be the same as calling all the indices individually for the same time: get(t, 0) ... get(t, n)

@jere8184
Copy link
Contributor

No erase method?

@heinezen
Copy link
Member Author

The array would have a fixed size from the start for the duration of its lifetime, so removing elements wouldn't make sense.

@jere8184
Copy link
Contributor

What about removing elements from keyframecontainers?

@heinezen
Copy link
Member Author

Is there a specific example where removing keyframes would be necessary? It's not used by any other curve. If we need it in the future, we could always add it, but I don't think it belongs to the public API of a curve array.

@jere8184
Copy link
Contributor

BaseCurve has an erase method, which is public.

@heinezen
Copy link
Member Author

@jere8184 Hmm... I think we could add it then. Certainly wouldn't hurt!

@jere8184
Copy link
Contributor

jere8184 commented Dec 26, 2024

How should the iterator handle KeyframeContainers that contain multiple keyframes for a given time? currently, we just iterate over a single valid keyframe for each container.

@heinezen
Copy link
Member Author

@jere8184 In that case, the last frame that was inserted (as in: by insertion order) for that time should be used.

@jere8184
Copy link
Contributor

So I've implemented the array curve using KeyframeContainers this differs from the other Container curves (queue and map) as they store their elements in element_wrappers this difference was supposedly because we will not erase Keyframes from the array right?

For the array curve, KeyframeContainers are fine because the size of the array is fixed, so all elements exist with a value over the duration of the array's existance. For the other containers, elements are not guaranteed to always be persistent. They can get erased and later added again, which has to be accounted for (that's why there's this dead/alive logic for their keyframes).

I'm just making sure I understand correctly before updating the curve documentation file.

Where does the idea that we shouldn't erase keyframes from array come from?

@heinezen
Copy link
Member Author

heinezen commented Jan 2, 2025

Where does the idea that we shouldn't erase keyframes from array come from?

I think that's more a misunderstanding. My point was that elements shouldn't be erased for arrays (in comparison to maps/quues), since the array always has a fixed size. Removing keyframes from specific containers is okay, it's just not a very common operation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: util Utilitis and data structures lang: c++ Done in C++ code nice new thing ☺ A new feature that was not there before
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants