Fix for #1015, #1170, and #950 #1214
Conversation
…lso looking for where bit layer files are for the seg fault when ctr-a on empty bitmap is called
Keeping local repo updated with master
…ew layer at the first keyframe
…og. Problem in MainWindow2.cpp line 1232
…og. Problem in MainWindow2.cpp line 1232
…lso looking for where bit layer files are for the seg fault when ctr-a on empty bitmap is called
…ew layer at the first keyframe
…og. Problem in MainWindow2.cpp line 1232
|
@ryyharris Awesome, thanks a lot! reviewers have been requested and will be take a look at it as soon as possible, but from a general overview I believe @candyface already fixed the issue with #950 with #1211 This specific commit 2a77bb1 worries me as it seems it would regress to the previous behavior that Pencil2D had, that is, not being able to delete the first keyframe of any layer, which was a limitation removed due to community feedback. This was fixed a long time ago but as i mentioned in the bug report, the problem seemed to be that it was only applied to the bitmap layers, so vector layers didn't catch up to that (hence the bug you found) It would be great if you could discuss this specific issue in detail with CandyFace as well, just in case i'm missing something here. |
scribblemaniac
left a comment
There was a problem hiding this comment.
Welcome and thanks for your contributions! There is quite a bit going on in this PR so I will try and break it down.
First off, it appears you may be having some difficulties with git. Your commits should under the same credentials as your github account so that they can be linked and whatnot. The binary files build/bin/pencil2d and build/tests/tests should not be uploaded, and I don't think there is a reason to change the permissions of main.cpp. If you need some help working out the contribution process, we would be more than happy to help you. The best way to reach the developers is on our official Discord server: https://discordapp.com/invite/8FxdV2g
Your fixes for #1015 and #1170 look good. We will probably cherry pick these changes out of this PR so that they can be included in the upcoming release.
As for the fix for #950, it's as @Jose-Moreno mentioned. This issue has already been fixed in a different manner. We did originally force a first frame, but it was frustrating to some of our users which is why we removed that restriction. We'll see what the rest of the team has to say, but I personally prefer @candyface's solution to this issue.
There are some commits of possible interest, such as the eyedropper issue or segfaulting on ctrl+a. These aren't issues that I am aware of, so if you could share some details on them that would be good. The changes that you have pushed unfortunately do not directly address these issues, but if you have fixes for them we would be interested in looking at that.
| #include "layercamera.h" | ||
| #include "platformhandler.h" | ||
|
|
||
| #include <cinttypes> |
There was a problem hiding this comment.
As far as I can tell, we don't need to add this header. Is there some error you are getting with your compiler that is resolved by this?
There was a problem hiding this comment.
I think something added the header from when I was trying to sync my branch with the latest in master since I never touched the headers.
|
|
||
| # use flags for gdb | ||
| #SOURCES += main.cpp | ||
| QMAKE_CXXFLAGS += -std=c++0x -g |
There was a problem hiding this comment.
You should use Qt's debugging mode rather than changing the flags. That's CONFIG+=qml_debug from the command line or just the debug build option in Qt Creator.
|
Thanks for the contribution @ryyharris :) I share the same thoughts and concerns as @scribblemaniac in regard to some of your commits. As for your addition of headers and flags:
Looks good to me too. |
|
Seems nobody has anything further to say about the #950 solution, so I think we're going to stick with using the current solution. Thank you for your work on it though and thanks again for your accepted contributions for the other issues. 👍 |
#1170 was fixed in the mainwindow2.cpp change.
#1015 was fixed by swapping the calls to next and previous layers made in scribblearea.cpp.
For #950, my fix is to prevent the error null keyframe state from ever occurring instead of addressing it in each function that would interact with the missing keyframe. My fix makes it so there will always be a first keyframe. Trying to delete it or move it will result in a new keyframe being placed at the beginning of the layer. The fix was based on how other animation applications, like Adobe Animate, handle trying to move the first keyframe.