Pencil: Segfault with the move tool

[scribblemaniac]

--Issue Summary--

Pencil2d crashes with a segmentation fault when clicking on the canvas under certain circumstances.

--Steps to Reproduce--

1. With a bitmap layer selected, create a new keyframe
2. Go to the first frame and delete it.
3. Click on the canvas with the move tool

--System Information--

• Pencil2D Version: 0.6.0

• Operating System: macOS 10.13.1

• RAM Size: 16 GB

--For Code Contributors--

The dispatch queue when the crash occurred was:

BitmapImage::copy(QRect)
BitmapImage::transformed(QRect, QTransform, bool)
ScribbleArea::applyTransformedSelection()
TimeLineCells::mousePressEvent(QMouseEvent*)

[CandyFace]

I can confirm this on Mac OS 10.12.6

[chchwy]

Crashed at ScribbleArea.cpp line 556

BitmapImage* bitmapImage = pLayerBitmap->getLastBitmapImageAtFrame(mEditor->currentFrame(), 0);
Q_CHECK_PTR(bitmapImage);

Two things probably need to do:

1. Pencil2D suppose a layer always has a key at frame 1 (not true for sound layers at the moment). So if a user wants to delete the key at frame 1, Pencil2D should automatically insert an empty new key after deleting the previous one.
2. Maybe pop up a warning dialog to tell users that they need to add a keyframe before drawing on it in the current time frame.

[CandyFace]

I think we can do better than a popup. UX wise that could become quite annoying if the user forget to add a new frame every so often.

A more intuitive solution would be to paint the whole canvas red when the layers are not sane, like this:

This would indicate that the frame does not exist and you therefore can't paint on the canvas.
What do you think about that idea?

[Jose-Moreno]

@chchwy for point 1. Have you seen moho / anime studio? it's a cut out animation software, you can rig on it and animate as well. Anyway they have a use for the frame 0 where they perform all the "posing" and once you get to frame 1 you cana ctually start animating.

If we extrapolate that idea we could leave the "initial" created keyframe on frame 0 as a base so it would always stay there.

As for point 2. I agree with the warning, and what CandyFace is suggesting isn't too far out so I agree with him that we should make it visible enough that you can't draw on the canvas if you don't have a keyframe container created.

@CandyFace Wouldn't be better if the rule was just not to allow the user to draw in the current layer if there is no keyframe at the current position? and change the tool icon to a "stop" sign? plus the warning "you can't draw on an empty frame, you need to create a keyframe"?

I understand what you're saying, but I think the colored area is a bit too much. Normally in other painting apps when you fill the screen with a color is to indicate that something will happen on that area.

In TVPaint for example when you fill an area you get a similar indicator to "preview" the filled zone, then you have to click again to fill it.

[Jose-Moreno]

@scribblemaniac Doesn't this problem come from the same root issue as #950 ? since it was fixed recently would you mind testing it again to see if we keep this issue open or closed once you have some time available? Thanks :slightly_smiling_face:

[chchwy]

I am not able to replicate this anymore with my win10 laptop.

[scribblemaniac]

Can't replicate the original issue, but a closely related one persists. If instead of trying to use the move tool, you try to select all (ctrl + a), it will still crash.

[CandyFace]

I am currently working on both the move and select tool, so I am assigning myself to this issue as there's still a bit of work to be done before I can merge my changes.

[Jose-Moreno]

[2019] Ctrl + A still crashes Pencil2D (0.6.4 and master) when the time indicator is located on the first frame without a keyframe.

[davidlamhauge]

Yeah... Ctrl+A still crashes Pencil2D.
In the SelectAll() function, the *bitmapimage that is returned by
BitmapImage *bitmapImage = static_cast<LayerBitmap*>layer)->getLastBitmapImageAtFrame(mFrame);
is a nullPtr, if the first frame is deleted. Same goes for the vector layer.
We can prevent this in two ways:
1) Prevent the user from deleting frame 1
2) Check for the nullPtr, and return if it is a nullPtr

I think preventing the user from deleting frame 1, is the best solution. Then they can draw on it, they can copy it, they can leave it blank, or whatever they want, but they shouldn't be able to delete it.

[CandyFace]

I don't see why the first frame should not be possible to delete, as long as there is at least one frame then I think it's fine. I'd prefer to check for null. We have had numerous amount of users asking/complaining why they were not able to delete the first frame, which is why you can delete it now.

[davidlamhauge]

I'm fine with both solutions. In fact I have a branch that adds the two lines of code needed, if we decide to check for nullPtr. Maybe @scribblemaniac should add it to his PR?
If we decide to let users delete first frame, then I've discovered another bug. Try this:
1) Open Pencil2D
2) scrub to frame 5 and make a keyframe.
3) scrub to frame 1 and delete the keyframe.
4) save the file.
5) open a new empty file or existing file.
6) open the first file again, and you'll discover that keyframe 1 had been reinstated.

[scribblemaniac]

I also don't like forcing there to be a frame at the beginning. We have discussed this previously I believe and the consensus was to allow it because we've already fixed multiple bugs related to this, such as the original issue here. If getLastBitmapImageAtFrame can possibly return nullptr (which is likely because it would be difficult to fully guarantee that there is always a frame at the beginning), then we should always be checking for a nullptr, regardless of which approach we take.

[chchwy]

Not very sure check nullptr everywhere is the best idea because of some of the design. I can't really remember where it is :\

Another solution is to put an empty guard frame at position 0 (The first frame is at position 1) so it will never be null.

[scribblemaniac]

Another solution is to put an empty guard frame at position 0 (The first frame is at position 1) so it will never be null.

This is a bad idea. If some one does manage to modify this frame (ex. keep drawing on previous option), then the users could be left with an unclearable start. Maybe it would onion skin that too. There's not telling how a frame at a position < 1 would save or how it would affect any place where we assume the keyframes start at 1. On the sound layer this would mean having an empty sound keyframe, which also causes issues.

Even if we found a way to guarantee that there is always a frame before the queried frame, the function could still potentially return a null pointer if the keyframe type is wrong (which would cause the static_cast to return nullptr) or it has been added improperly and is actually just a nullptr. Neither of these situations are not particularly likely now, but I could see the former happening when we allow frames to be moved between layers. Having a check for nullptr is good practice in any case.