-
Notifications
You must be signed in to change notification settings - Fork 275
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
Xsheet feature #1119
base: master
Are you sure you want to change the base?
Xsheet feature #1119
Conversation
For the reviewer(-s): The first ten commits are from the time when the xsheet was a dialog alwaysOnTop. In the 11th commit I have made it dockable. Much of the code in the first 10 commits are non-existent. |
This should now work no matter why a cell is selected: mouse click, mouse drag, arrow keys, tab, etc.
Changes: - Underline button labels - Remove ':' from button labels - Add left and right margin for buttons - Remove extra horizontal spacers - Capitalize CSV (acronym) - Change xsheet to x-sheet. This seems to be the more common spelling online - Remove spacing between label and buttons
Xsheet interface tweaks
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 have started to review this, but have not tested or look through everything in the x-sheet .ui and .cpp files yet. I've fixed a couple of things myself which you should double check, and I've made a comment below. Only cosmetic things, everything has been working more or less as expected.
There is a big problem though. The x-sheet is noticeably slowing down scrubbing and playback for me. Just clicking and dragging on the timeline has a lot more lag, and if you change the fps to a high value like 60, there is a noticeable difference in the playback speed. Looking at the code, you're regenerating the whole x-sheet table each time which involves multiple large loops over every cell. Ideally the x-sheet should only be generated at the beginning and should apply only the changes as they are made. At the very least, this "delta update" approach should be used for scrubbing so that it does not affect playback performance.
@@ -196,6 +200,7 @@ void MainWindow2::createDockWidgets() | |||
addDockWidget(Qt::RightDockWidgetArea, mColorBox); | |||
addDockWidget(Qt::RightDockWidgetArea, mColorInspector); | |||
addDockWidget(Qt::RightDockWidgetArea, mColorPalette); | |||
addDockWidget(Qt::RightDockWidgetArea, mXsheet); |
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 don't think the xsheet should be displayed by default. It will clutter the interface and cause confusion for new animators. Professional animators are the ones that will mainly benefit from this, and I think they should be more than capable of enabling it in from windows menu. Will see what they others have to say about this too.
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 agree that the xsheet should be hidden by default. Pencil2D should keep its clean and easily accessible interface.
I also agree that the xsheet should be updated more efficient. I know there is a wish that the frames should be visible (in miniature) in the xsheet, so it will be corrected. That said, I think 60 fps or more i only seen in games, where it is common to have 50-100 fps, but that depends on cpu power. Hand drawn animation is max 24 or 25 fps in Europe and 30 fps in the States. Correct me if I'm wrong.
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'm glad we agree. You would have to be crazy to do frame-by-frame animating in 60 fps, though maybe someone is importing a 60 fps video and rotoscoping on twos or something along those lines. It might also be useful for smoother camera motion since that does not require any extra work for the user. I was mainly using that as an example to demonstrate the issue more clearly. We've had many complaints about our playback speed so we should avoid compounding the issue with more contributing factors.
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've solved the problems mentioned. Rewrites of xsheet is now limited to situations where it is necessary.
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.
@scribblemaniac I have made changes, so I use the layer index in stead of layer name as identifier. This means that identical layer names are supported now.
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.
Overall I think it looks good, however it should be noted that there's a performance penalty of updating both the timeline and xsheet and it's pretty significant imo. All signals are emitted no matter if the window is visible/hidden or not. That means that the Xsheet is constantly updated, even if we don't use it.
We should look into disconnecting widgets whenever they are hidden and re-establish connection when visible.
I see what you mean @candyface .
Would this solve the problem? |
@davidlamhauge That solution only solves half the problem and is not exactly pretty if it has to be added to all methods that receives a signal.
will still be emitted, which will make unnecessary calls to the methods, even if they're returned at the beginning. A better and more long term solution would be to have a signal that is emitted whenever the widget is opened and closed. A signal can be broken by using disconnect(...) |
I can see your point. The QDockWidget class has a signal visibilityChanged(bool visible) with the following description: "This signal is emitted when the dock widget becomes visible (or invisible). This happens when the widget is hidden or shown, as well as when it is docked in a tabbed dock area and its tab becomes selected or unselected." |
No I wouldn't say the approval is dependent on this, that's why I approved the PR in the first place ;) This will have to be implemented in another PR. The approach required to get this implemented correctly will end up creating conflicts in all PR's though... so I'd like to see most of them being merged before I start making such a pull request. |
Transcript snippet from our last meeting in case people (me) forgets why we haven't merged this yet...
|
I got some free time recently and revisited this PR today. I think it's pretty close to being merged. Just need some cleanup. There are a couple of things I want to clarify:
Thanks! |
@chchwy Hi! Here are some answers while David and the others chime in:
Additionally I found this video from David explaining the some of the code functions and how the feature worked in case it helps. Hope this helps. Cheers. |
Thanks for your detailed explanation @Jose-Moreno I believe it will be easier for us to make progress. one bite at a time. |
This xsheet is to great extent a vertical replica of the timeline.
It shows the drawable layers, unless they are hidden. Lipsync data can be added or imported from Papagayo file or saved/loaded from native file import (.lip2d). Xsheet content can also be exported as Comma Separated Values (.csv).
It suits many animators workflow, but has still the same simplicity that is aimed for in the Pencil2D community.