Animation timeline improvements and added "empty frame" concept to data model
AbandonedPublic
Actions

Authored by scottpetrovic on May 3 2018, 3:09 PM.

Details

Reviewers

dkazakov
emmetoneill

Group Reviewers

Krita

Maniphest Tasks

T8652: Krita 4.1.4 Release Task

Summary

This patch does a few things...

Introduces the concept of an 'empty frame' to the data. This is just a simple calculation to see if the width of the layer contents is 0
Based off that data change, the timeline docker can show if a keyframe has content in it or not.

It also has a display of a "hold" frame. If the frame is part of a hold frame, it will show a line through the frame

I also added a slightly color change to a row if it is selected. The active row gets things applied to it so I think it needs to be called out more than it is right now on the left side by the layer name.

There are quite a few more states now to how the color is shown, so I refactored that color calculation and turned it into multiple passes to make it easier to read.

There is a "hack" that I feel needs to be looked at. The timeline display won't repaint itself, so I ended up quickly changing the current time back and forth to force a refresh. Maybe there is a better way to do it.

Test Plan

I tried adding keyframes
changing the empty/filled state with painting on the canvas
clear the canvas to make it blank
move keyframes around
have multiple layers to see how they would update

Diff Detail

Repository

R37 Krita

Lint

Lint Skipped

Unit

Unit Tests Skipped

scottpetrovic requested review of this revision.May 3 2018, 3:09 PM

scottpetrovic created this revision.

scottpetrovic edited the summary of this revision. (Show Details)May 3 2018, 3:21 PM

I am going to have to work on this a little bit. When I create vector layers with this patch krita crashes...I think there is something it doesn't like about calculating the contents on vector layers.

Hi, @scottpetrovic!

You have changed the color of the color labels :) The new default color label is invisible on the default dark color theme :)

There is also a one-pixel offset in the crossing-line

I'm not sure I like the color of the crossing line. The color coincides with the "selection color", so I expect it to show some selection, but it is not a selection. Probably, you can use the keyframe color for that line? Or it will be too contrast?

In general the idea is nice, but I would like to hear painter's opinion about that. Can it be that the timeline will become too contrast and disturbing?

I didn't check code too much, since it is going to be changed. Just noticed that there is some weird TimelineFramesModeL::setHasContent() function. I'm not sure it is needed. I would imagine that KisKeyframeChannel would just have a method bool hasContent(int time) const and the model would just request that data when needed. I don't know why there is a special setHasContent() call :)

plugins/dockers/animation/timeline_frames_model.cpp
157	I'm not sure what this function does? The value seem to be calculated from !layerDevice->extent().isEmpty(), why may anyone want to set this value?

Thanks for the quick testing/review dmitry

I fixed my vector crash and I think I have your first set of points taken care of. Blank frames with no color labels should be getting a border now. The hold frames should inherit from their color from their keyframe. I don't see a small line extending at the end of a hold frame so I think that is taken care of too.

In terms of the actual code refactoring...I am not quite sure how to remove the setHasContent. It is currently here...

TimelineFramesModel::setData(...)

This is currently the trigger for when a frame is updating its data. That setHasContent() call is in there. Removing it causes the frames to not update right. Maybe I need to update the frame data differently?

scottpetrovic added inline comments.May 7 2018, 7:40 PM

plugins/dockers/animation/timeline_frames_model.cpp
157	This is only used once for this set function. TimelineFramesModel::setData(...) setData is the trigger that updates the frame data with this hasContent logic. Maybe there is a better place to call this when updating the state of the frame data

Hi, @scottpetrovic!

I have made a patch that fixes some design problems, please see it in P210 (should be applied on the top of your patch). What it does:

Implement lazy calculation of KisKeyframe::hasContent(). Now noone should set/update its values, it calculates the result on the fly (by asking its parent keyframe channel)
Fix the updates of has-content status of the timeline. I just connected KisImage::sigImageModified() signal to the model and emit "dummy-update" request when needed (which is already compressed).

There are two small regressions that I managed to find in the patch, please find them in the inlined comments :)

plugins/dockers/animation/timeline_frames_item_delegate.cpp
78	`framePresent` and `active` variables are now unused. I guess it should somehow break rendering of the opacity frames...
113	`isEditable` variable is now unused. It means that the layer doesn't change its color when is becomes invisible/locked, which is a regression.

This revision now requires changes to proceed.May 10 2018, 4:30 PM

I applied to your patch to the branch and also fixed up a couple of the regressions. Let me know if there are any other issues with this.

thanks for the review

Love the patch so far, the animation tools just keep getting better!

I do have one small visual nitpick. With the addition of empty/blank frames, it's slightly hard to see any difference under the scrubber on the timeline when you're created a new frame:

On the left you can see the scrubber selecting frame 30 without any frame. On the right side is what the user sees after they've hit "create blank frame".
The current thin outline looks great when the frame isn't currently selected, but it's nearly impossible to see under the scrubber, even with decent eyesight. And, as a result, the visual feedback isn't great when you create a new frame, in my opinion.

There are a few subtle ways that this might be able to be solved:

Change the color of the little dot in the center of scrubber to the same color as the keyframe that it's over. (This would make the dot in the right picture blue)
Change the shape of that dot to a little X whenever the scrubber is not over a keyframe. (The dot would be an X in the left picture)
Remove the orange tint under the scrubber when it's over an empty/blank frame. (There would be no orange tint in the right picture)
Change the color of the tint under the scrubber to a semi-transparent version of the color of the empty frame that it's on top of. (The tint under the scrubber would be blue instead of orange in the right picture)
Make the empty/blank frame outline thicker or bolder when it's currently selected by the scrubber.
Add some diagonal hatching or other pattern to the empty/blank frames to make them more visible under the scrubber selection.

It's possible that this doesn't bother anybody but me, but I felt that it's worth mentioning anyway. Keep up the good work!

Hi, @scottpetrovic!

From my side the patch looks fine now :) The only wish I have is to dim the contrast of the empty frame placeholder line a bit. But it is not critical. I'm okay with fixing it later.

Please get an approval from @emmetoneill (adding him as a blocking reviewer) and push it :)

@emmetoneill - There are some good ideas with how the active column and row are displayed.

Would you be ok if I push this out as-is for now since it is approved. We have a pro animator that needs to play around with what I have. He is waiting for this to be able to give his feedback on how it looks.

After this is out there, we all can discuss any visual changes for a future patch. Those will be pretty simple to code compared to what is in this thing.

Oh yeah, totally, push it as soon as possible!

I just wanted to make note of a small visual nitpick that felt odd to me when testing out the patch. It's definitely not a showstopper.

emmetoneill accepted this revision.May 12 2018, 5:20 PM

This is pushed out. Once Bollebib also plays around with these changes a bit...we can gather everyone's feedback with exactly what type of styling this needs to have and put it in a separate task.

woltherav added a task: T8652: Krita 4.1.4 Release Task.May 18 2018, 10:40 AM

Revision Contents
Changeset List

			Path	Packages
M			libs/image/kis_base_node.h (13 lines)
M			libs/image/kis_base_node.cpp (17 lines)
M			libs/image/kis_keyframe.h (2 lines)
M			libs/image/kis_keyframe.cpp (11 lines)
M			libs/image/kis_keyframe_channel.cpp (6 lines)
M			libs/image/kis_layer_properties_icons.h (1 line)
M			libs/libkis/Node.h (7 lines)
M			libs/libkis/Node.cpp (11 lines)
M			plugins/dockers/animation/kis_time_based_item_model.h (6 lines)
M			plugins/dockers/animation/timeline_color_scheme.h (2 lines)
M			plugins/dockers/animation/timeline_color_scheme.cpp (18 lines)
M			plugins/dockers/animation/timeline_docker.h (2 lines)
M			plugins/dockers/animation/timeline_docker.cpp (36 lines)
M			plugins/dockers/animation/timeline_frames_item_delegate.h (3 lines)
M			plugins/dockers/animation/timeline_frames_item_delegate.cpp (95 lines)
M			plugins/dockers/animation/timeline_frames_model.cpp (65 lines)
M			plugins/dockers/animation/timeline_frames_view.cpp (1 line)