-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
base: master
Are you sure you want to change the base?
Array curve #1724
Conversation
3ea0f4a
to
4c2b2f3
Compare
is there any chance array curve should be an array of base curves instead of an array of key frame containers? |
4c2b2f3
to
0b01820
Compare
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. |
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, |
0b01820
to
1d653cd
Compare
f68ec30
to
0e637b0
Compare
Some additional remarks:
|
alright, currently implementing the changes, really appreciate all the feedback. |
22d7740
to
dda1518
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found everything I wanted to comment on :D
} | ||
|
||
// Array::Iterator is used to iterate over KeyframeContainers contained in a curve at a given time. | ||
class Iterator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why this class doesn't implement CurveIterator
and uses a different strategy for iterating?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One reason would be that std::_Array_const_iterator
doesn't have base
as a member, which is referred to in iterator.h line:95
, and thus unable to inherit from CurveIterator
But to be honest when the choice was made it was arbitrary.
Closes #1678