Skip to content
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

Fix pressure artifacts at end of brush stroke #1155

Merged
merged 1 commit into from Jan 22, 2019

Conversation

Projects
None yet
3 participants
@scribblemaniac
Copy link
Member

scribblemaniac commented Jan 6, 2019

Both tablet and mouse events were getting called during a brush stroke. Don't ask me why, because I don't know. Qt 5.12 did not fix this issue. Setting Qt::AA_SynthesizeMouseForUnhandledTabletEvents to false also had not effect. Since the tablet press occurs consistently before the mouse press, we can work around this issue by disabling part of the mouse events when the tablet is in the middle of a brush stroke.

Fixes #677. Possibly relates to #1154.

Note: If this code is used for ScribbleArea::mouseMoveEvent, there is a significant degradation of stroke quality on computers affected by this bug:

void ScribbleArea::mouseMoveEvent(QMouseEvent* event)
{
    updateCanvasCursor();

    if (!areLayersSane())
    {
        return;
    }

    Q_EMIT refreshPreview();

    // Workaround for tablet issue (#677 part 2)
    if(!mMouseInUse) return;

    mStrokeManager->mouseMoveEvent(event);
    mCurrentPixel = currentTool()->getCurrentPixel();
    mCurrentPoint = currentTool()->getCurrentPoint();

    // the user is also pressing the mouse (= dragging)
    if (event->buttons() & Qt::LeftButton || event->buttons() & Qt::RightButton)
    {
        mOffset = getCurrentOffset();
        // --- use SHIFT + drag to resize WIDTH / use CTRL + drag to resize FEATHER ---
        if (currentTool()->isAdjusting)
        {
            currentTool()->adjustCursor(mOffset.x(), event->modifiers()); //updates cursors given org width or feather and x
            return;
        }
    }

    if (event->buttons() == Qt::RightButton)
    {
        getTool(HAND)->mouseMoveEvent(event);
        return;
    }

    if (currentTool()->isDrawingTool())
    {
        if (event->buttons() & Qt::LeftButton)
        {
            currentTool()->mouseMoveEvent(event);
        }
    }
    else
    {
        // MOVE, SELECT OR HAND TOOL
        currentTool()->mouseMoveEvent(event);
    }

#ifdef DEBUG_FPS
    if (mMouseInUse)
    {
        clock_t curTime = clock();
        mDebugTimeQue.push_back(curTime);

        while (mDebugTimeQue.size() > 30)
        {
            mDebugTimeQue.pop_front();
        }

        if (mDebugTimeQue.size() > 30)
        {
            clock_t interval = mDebugTimeQue.back() - mDebugTimeQue.front();
            double fps = mDebugTimeQue.size() / ((double)interval) * CLOCKS_PER_SEC;
            qDebug() << fps;
        }
    }
#endif
}

This makes me think that the mouse move events being received when the tablet is in use are actually unique events and not duplicate/synthesized events. Note that with the the code in this PR, the mouse events are getting added to the stroke manager, but not to the tool. Not sure if this is the best way to do it. I think blocking mouseReleaseEvent is the critical part of this fix.

@scribblemaniac scribblemaniac added this to the 0.6.3 milestone Jan 6, 2019

Fix pressure artifacts at end of brush stroke
Both tablet and mouse events were getting called during a brush
stroke. Since the tablet press occurs consistently before the
mouse press, we can work around this issue by disabling part of
the mouse events when the tablet is in the middle of a brush
stroke.

@scribblemaniac scribblemaniac force-pushed the scribblemaniac:reed-fix branch from 620fceb to 7bf0601 Jan 6, 2019

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Jan 6, 2019

@scribblemaniac Could you please check one thing?

I remember that Qt will resend a mouse event if no one accepts the tablet event (calling event->accept()).
I just did a quick glance at the tablet event handling and didn't see any tablet event accepted.

@scribblemaniac

This comment has been minimized.

Copy link
Member Author

scribblemaniac commented Jan 6, 2019

I remember that Qt will resend a mouse event if no one accepts the tablet event (calling event->accept()).

This was one of my first thoughts, however when I looked into it, event->accept() is getting called at the bottom of ScribbleArea::tabletEvent, and I see absolutely no way that that function is returning before accepting the event short of an exception. Not to mention that we should be able to replicate this on more systems if that was the issue.

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Jan 6, 2019

I didn't see the event->accept(), can you tell me where it is?

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Jan 6, 2019

Oh sorry never mind. My bad.

@CandyFace
Copy link
Member

CandyFace left a comment

Tested with both my mouse and tablet.
I've never had this problem though, my testing was merely done to make sure that the bugfix didn't break anything.

@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Jan 7, 2019

before patch
image

after patch
image

There seems to be virtually no difference between the strokes imho. Through my experimentations, it all comes down to the size of the drawings, ie. a bottleneck of the canvas not updating fast enough, thus halting the tablet events.

edit: further more, I've tested the mouse and tablet events, trying to sample each and i'm seeing very little difference if non at all between the two, except if I disable coalescing on mac os...

@scribblemaniac

This comment has been minimized.

Copy link
Member Author

scribblemaniac commented Jan 8, 2019

@CandyFace Thanks for testing, I'm glad to hear everything is working.

I don't know if you stayed to the end of @Jose-Moreno 's livestream for this issue but we made some discoveries then. There seemed to be a noticeable degradation in quality when compared to the master branch (even when he compiled that himself rather than using the nightly build). However this issue was fixed after I changed ScribbleArea::mouseMoveEvent from the code above to the code in this PR. I can only think that extra events for mStrokeManager helped somehow. I personally don't experience any difference with my tablet for either version of the code, so it's probably specific to users who were experiencing the original issue.

@CandyFace

This comment has been minimized.

Copy link
Member

CandyFace commented Jan 11, 2019

Hmm yes it definitely looks like it is affected in mousemove event... that's so strange. I did a quick measurement of the events before and after the change in mousemove but I see no difference in amount of samples between tablet and mouse.

I guess we'll have to dig deeper to find the reason why this happens.

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Jan 22, 2019

Tested.
Found another issue, bucket tool doesn't work with tablet as it works with mouse.
Well done @scribblemaniac

@chchwy chchwy merged commit 9618526 into pencil2d:master Jan 22, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.