-
Notifications
You must be signed in to change notification settings - Fork 205
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
AnimationGadget : fix crash related to dragging keys on adjacent frames #2720
Conversation
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.
Hi Matti,
Good detective work! I think the change of sorting criteria for m_selectedKeys
is problematic though :
- Keys from different curves, but with the same time, are now considered identical, but they're not. Among other things, this causes problems when trying to drag two curves at the same time.
- Changing the time of a key means the selection is no longer sorted, which breaks subsequent set operations. Among other things, this breaks the selection in one AnimationGadget if you move just a subset of it using another AnimationGadget (or via undo, or any other operation).
Basically, using the time breaks the requirements of std::set
. The main reason for KeyPtr
to exist at all was to allow the AnimationGadget to have a consistent handle for each key. The selection is all about key identity (rather than value), so it should continue to sort by address as before. That means we'll need a different approach to the bugfix. Seems like these might be options :
- Don't assume the key has a parent. If it doesn't, re-aquire the parent by querying
m_overwrittenKeys
, and add the key back. - Make a temporary
vector<Key *>
in each call tomoveKeyframesInternal()
, sorted by time, either ascending or descending depending on the offset. Iterate that.
I'd expect 1 to be quicker in the ideal case of no clashes, but 2 to be quicker in the worst case of everything clashing. I'd be inclined to go for 2 first though, because 1 introduces addKey()/removeKey()
calls that aren't really reflective of what we're trying to achieve - it puts the Animation::CurvePlug into strange intermediate states.
Hope that makes sense...
Cheers...
John
include/GafferUI/AnimationGadget.h
Outdated
std::set<Gaffer::Animation::KeyPtr> m_selectedKeys; | ||
struct CompareKeyByTime | ||
{ | ||
bool operator()(const Gaffer::Animation::KeyPtr lhs, const Gaffer::Animation::KeyPtr rhs) const |
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.
would be better to pass KeyPtr
by reference rather than by value, to avoid unnecessary increment/decrement of reference count.
src/GafferUI/AnimationGadget.cpp
Outdated
for( auto &key : m_selectedKeys ) | ||
V2f offset( xSnappingCurrentOffset - m_xSnappingPreviousOffset, currentDragPosition.y - m_lastDragPosition.y ); | ||
|
||
if( offset.x < 0 ) |
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.
This could definitely do with a comment explaining the reason for the different ordering.
92fb1e2
to
902dd5a
Compare
Hey John, I believe I've implemented something very close to (2) now. Could you have a look? Thanks! Matti. |
902dd5a
to
ae388c9
Compare
ae388c9
to
672c737
Compare
Updated. Removed the templated little helper function and moved the sorting logic into the lambda. |
Thanks Matti - merged manually after fixing a small formatting problem... |
Hey John,
I've spent some time today looking into the crash that Dan reported and think I've finally been able to fix it.
What happened: When moving keys on adjacent frames, if the order in which we processed keys was wrong, we removed an existing key
A
because another keyB
was moved toA
's location in time, but then processedA
as part of the current selection and relied on its parent being valid. The key didn't have a parent at that time though, and Gaffer crashed. Looks like I just never tested with keys on adjacent frames...What I've done now to fix it is to determine the order in which we process keys by the direction in which the user wants to drag them. Can you have a look at what I've done? The whole thing with a templatized, internal method might be a little clunky, but it allows me to easily reverse the iteration direction by using reverse_iterators. I can look into making that a free function that accepts
m_overwrittenKeys
as an argument if you find it annoying to have that little templatized helper show up in the header.After doing that, it took me a little bit to figure out the second part of the puzzle: I had a test scene that worked when building with DEBUG=1, but a release build would always crash. It seems that when doing a debug build, we can rely on
Keys
getting created in order. With compiler optimizations turned on, that doesn't seem to be the case anymore. What bit me is that we sorted keys by memory address instead of their time value. I've fixed that, as well.Would you have a look to see if what I've done makes sense?
Cheerio,
Matti.