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

Pegbar alignment #1205

Merged
merged 22 commits into from Jul 25, 2019
Merged

Pegbar alignment #1205

merged 22 commits into from Jul 25, 2019

Conversation

davidlamhauge
Copy link
Contributor

@davidlamhauge davidlamhauge commented Mar 30, 2019

A feature relevant for animators that animate on paper and import their scanned drawings into Pencil2D. If they use the scanners document feeder, they can expect som inaccuracies, so their artwork is not aligned.
This feature alignes the center pegbar of the drawings.
Usage:

  1. select the keyframe that is to be the point of reference. This is often a field guide.
  2. With the select tool, select an area around the center peg hole, that encircles all center peg holes.
  3. Select peg bar alignment in the menu.
    The artwork is now horizontally and vertically aligned.

}
}
}
return 10000;
Copy link
Member

@MrStevns MrStevns Apr 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should return -1 here instead of 10000.

Copy link
Contributor Author

@davidlamhauge davidlamhauge Apr 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function 'findLeft' searches for, and returns, the left-most x-value of the peg bar you have selected. This value could in some cases be -1, which is pretty close to the center of the canvas. That's why I chose 10000 as a return value.

Copy link
Member

@MrStevns MrStevns Apr 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... in that case I'd prefer if we could maybe settle on another return type. Given that the coordinate can be anything, even 10000, returning a custom value that can hold both the intended value but also a potential Errorcode would be better and easier to understand for people trying to read the code too.

So it could be something like...

... will be defined in pencilerror.h
struct StatusInt {
        int value = 0;
        ErrorCode errorcode;
    };

and then return that value in your findLeft and findTop top function with an appropriate errorcode.

Now you can simply return a StatusInt and catch the appropriate errorcode instead.
This could come in handy in many other places, so I think that would be a nice addition to the project.

Copy link
Contributor Author

@davidlamhauge davidlamhauge Apr 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have never made a struct before, but I can see that it often is defined as a class.
However, I can't get it to work unless I put it inside the Status class, after the ErrorCode Enum, like this:

        ERROR_NEED_AT_LEAST_ONE_CAMERA_LAYER
    };

    struct StatusInt {
        int value = 0;
        ErrorCode errorcode;
    };

Is that the way to do it? It must somehow know what ErrorCode is?

Copy link
Member

@MrStevns MrStevns Apr 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's correct, the struct needs to know the origin of the values inside it. Putting the struct above Errorcode isn't allowed because ErrorCode doesn't exist yet.

}
}
}
return 10000;
Copy link
Member

@MrStevns MrStevns Apr 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should return -1 here instead of 10000.

Copy link
Contributor Author

@davidlamhauge davidlamhauge Apr 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above...

@@ -44,7 +44,11 @@ GNU General Public License for more details.
#include "soundmanager.h"
#include "viewmanager.h"

#include "layer.h"
Copy link
Member

@MrStevns MrStevns Apr 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core files should not exist in app. Only widgets and UI related should exist here.

#include "layercamera.h"
#include "layerbitmap.h"
Copy link
Member

@MrStevns MrStevns Apr 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core files should not exist in app. Only widgets and UI related should exist here.

#include "layercamera.h"
#include "layerbitmap.h"
#include "bitmapimage.h"
Copy link
Member

@MrStevns MrStevns Apr 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core files should not exist in app. Only widgets and UI related should exist here.

Copy link
Member

@MrStevns MrStevns Apr 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to remove the header files again that you don't use.

Copy link
Member

@MrStevns MrStevns left a comment

Although the feature itself looks good, the code in mainwindow2 will have to be moved elsewhere as you're mixing core_lib and app which is not allowed.

Anything that modifies content and the like without interacting through gui should be move to core_lib instead, you'll have to split up the code so some lies in Editor, then calls the respective classes to make the required changes.

        for (int i = 0; i < sl->count(); i++)
        {
            layerbitmap = static_cast<LayerBitmap*>(mEditor->layers()->findLayerByName(sl->at(i)));
            for (int k = layerbitmap->firstKeyFramePosition(); k <= layerbitmap->getMaxKeyFramePosition(); k++)
            {
                if (layerbitmap->keyExists(k))
                {
                    img = layerbitmap->getBitmapImageAtFrame(k);
                    int tmp_x = img->findLeft(rect, 121);
                    if (tmp_x == 10000)
                    {
                        QMessageBox::information(this, nullptr,
                                                 tr("Peg bar not found at %1, %2").arg(layerbitmap->name()).arg(k),
                                                 QMessageBox::Ok);
                        return;
                    }
                    int tmp_y = img->findTop(rect, 121);
                    img->moveTopLeft(QPoint(img->left() + (peg_x - tmp_x), img->top() + (peg_y - tmp_y)));
                }
            }
        }

I'd make this its own method, calling it adjustContentOfLayer and then another adjustContentOfLayers that calls adjustOfContentOfLayer but with the for loop integrated.

The QMessage boxes will still have to be in the widget though so you'll have to make return values that can execute that still if the alignment fails etc...

Remember that for each time you implement something, it should be possible to write tests for it that doesn't rely on UI. so adjusting layer content should be 100% independent of widgets.

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Apr 17, 2019

I've separated the functions, so the core functionality is in the core-lib. I've tested it as good as I can, and it works fine.
One day I would like to know how you "...write tests for it that doesn't rely on UI." @candyface

Copy link
Member

@MrStevns MrStevns left a comment

LGTM now aside from a few comments. Although I think an introduction to peg bar registration is needed for new users but that can probably wait till some other time. It's a nice to have thing.

app/app.pro Outdated
@@ -89,7 +90,8 @@ SOURCES += \
src/spinslider.cpp \
src/doubleprogressdialog.cpp \
src/colorslider.cpp \
src/checkupdatesdialog.cpp
src/checkupdatesdialog.cpp \
src/pegbarregistration.cpp
Copy link
Member

@MrStevns MrStevns Apr 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use same name convention as the other widget classes, ie. pegbarregistrationdialog because it's a dialog widget.

app/src/mainwindow2.cpp Outdated Show resolved Hide resolved
#include "layercamera.h"
#include "layerbitmap.h"
#include "bitmapimage.h"
Copy link
Member

@MrStevns MrStevns Apr 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to remove the header files again that you don't use.

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Apr 20, 2019

I have removed unused headers, and renamed the class to the convention.
It compiles and runs fine, but somehow the checks failed?

@MrStevns
Copy link
Member

MrStevns commented Apr 20, 2019

You forgot to rename the _ui file.

../../app/src/pegbaralignmentdialog.cpp:2:35: fatal error: ui_pegbarregistration.h: No such file or directory
 #include "ui_pegbarregistration.h"

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Apr 22, 2019

I finally got it renamed - I hope...

@MrStevns MrStevns requested review from scribblemaniac and chchwy Jul 1, 2019
@MrStevns
Copy link
Member

MrStevns commented Jul 1, 2019

I'll be merging this tomorrow if there are no further comments.

@Jose-Moreno
Copy link
Member

Jose-Moreno commented Jul 1, 2019

@candyface I can't build David's branch to test right now, it seems to give me a few errors for missing methods on ScribbleArea (i.e deselectAll() and getSelection() )

I'll wait until it's merged to test it as there were some issues I presented to David before and I think they are specific to windows since last time he told me he wasn't getting them at all with the same test resources (e.g drawings were not being properly aligned on first pass, zooming affected alignment, operating on already aligned would mess up the alignment instead of preserving it)

@scribblemaniac
Copy link
Member

scribblemaniac commented Jul 1, 2019

Sounds like there might be some merging issues from what @Jose-Moreno said. Additionally, I would like to review this before it gets merged. It shouldn't take too long so please hold out on merging this right away.

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Jul 1, 2019

I have made the changes so it can compile now.
Tested it on Ubuntu, and it works as expected.

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Jul 11, 2019

On discord we've (@Jose-Moreno , @candyface and @scribblemaniac ) had a discussion whether it would be an enhancement if the dialog was modal. With the last merge the dialog is modal. So far just checked on Ubuntu.
As @scribblemaniac suggested it was a challenge programming wise, but with some tweaking I got it to work :-)

@MrStevns
Copy link
Member

MrStevns commented Jul 13, 2019

Two quick comments from your observations in discord:

  1. You are able to open several alignment-dialogs. This should be prevented.

This can be prevented by adding a reference to the header and seeing if it exists already, quick fix.

2.2) Apparently there are some issues on the modal thing. In Ubuntu it worked fine, and I could interact with the UI, despite of the dialog. On my MacBook the dialog does not stay on top, but disappears under the MainWindow...?

Add setWindowFlag(Qt::WindowStaysOnTopHint); to your dialog window, this will make it stay on top if it's possible. Works on Mac OS.

Copy link
Member

@MrStevns MrStevns left a comment

Functionality wise I think it looks good, my request of changes are mainly to avoid bloating mainwindow2 with unrelated methods. Additionally there are some rename suggestions and removal of GUI feedback in Editor but aside from that I think it looks good.

This will be a very nice addition to the program for those that rely on imports, great job :)

pegreg = nullptr;
}

void MainWindow2::updatePegRegLayers()
Copy link
Member

@MrStevns MrStevns Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should be moved into the pegbar class. If you need to access Editor and related core functionality, add a void setCore(Editor*) in your header and keep a reference of Editor named "mEditor". Alternatively you can add it as parameter in the constructor.

pegreg->setLayerList(bitmaplayers);
}

void MainWindow2::updatePegReg()
Copy link
Member

@MrStevns MrStevns Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This too is only related to pegbar so it should moved to that class.

@@ -1397,3 +1492,27 @@ void MainWindow2::changePlayState(bool isPlaying)
}
update();
}

void MainWindow2::alignPegs()
Copy link
Member

@MrStevns MrStevns Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, move to pegbar class.

@@ -65,6 +66,10 @@ class MainWindow2 : public QMainWindow
void undoActSetEnabled();
void updateSaveState();
void clearRecentFilesList();
void pegBarReg();
Copy link
Member

@MrStevns MrStevns Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent of the function could be clearer, we're launching a dialog, so the method could be called openPegBarRegDialog or pegBarDialogPressed or similar.

retLeft = img->findLeft(rect, 121);
if (retLeft.errorcode == Status::FAIL)
{
QMessageBox::information(nullptr, nullptr,
Copy link
Member

@MrStevns MrStevns Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand what you want to do here but Editor and Core in general should not contain GUI feedback because in case a test is made, the program will get stuck if something like a qmessagebox is triggered. Although we don't have many tests for now, it should always be possible to run a method when implementing in core, without triggering GUI feedback.

I think it would be a good idea to have a general error or warning popup that can be emitted from Core and accepted in Mainwindow2. I'll make a PR on your branch.

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Jul 16, 2019

I have adressed all your requests @candyface.
I forgot to erase some marked out code in MainWindow, but I'll wait to commit this small change, until I know if there will be more requests. Thanks for helping out!

Copy link
Member

@MrStevns MrStevns left a comment

@davidlamhauge Previously I've mainly focused on reviewing the code, now I'll comment on the UI/UX.

image

  1. There's a bug that if you close the dialog via the window frame (pressing the cross), then you can't open the dialog again. You'll be told it's already open.

  2. Usability wise I don't like that the user doesn't know that the dialog disappears when they've pressed align, I'd rather see the dialog being kept open and the user can choose to close it whenever they feel happy with the result. You could rename the cancel to "Close", so the choices would be "close" ---- "align", then it's fully up to the user whether they want to close or keep refining.

  3. The default state is currently that cancel will be highlighted, this should be "Align Peg Bars"

image

You can change it like by doing the following:

image

  1. Cancel is misleading since there's nothing to cancel, clicking on cancel simply closes the dialog again. If this is the intention then it should be named "close" instead, otherwise it should reverse its action.

  2. "pegbar alignment failed!" doesn't give me much indication of what is wrong (in my case I tried to align white pegbars), why did it fail or what can I do to prevent it? Although there are prerequisites, the error message should still try to explain the situation.
    A better error message could be "Was not able to find pegholes, make sure you've selected all of them and try again"

Future work

There's room for further improvement. The aligner only works with pegholes that are black, which might not always be the case. Often the result is still offset by one pixel or so but overall it works quite well.

Code wise it looks good to me, well except that bug :)

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Jul 23, 2019

Thanks for the comments @candyface .

  1. The only way I can eliminate the close-button bug, is to disable the close button in the upper corner. If you know another way, please let me know.
  2. The feature looks for peg holes that has a gray value of 121 or less, so peg holes could be red, blue or other colors, but they should have a tone that is medium gray or darker. We could add it as a fourth prerequisite and/or the level could be slightly higher (150-200), but we should not look for white pegholes. Nobody else does, and I've never heard of it before.

Copy link
Member

@MrStevns MrStevns left a comment

Looks good to me now! Well done @davidlamhauge and thanks for being patient :)

I'll merge it later if there are no further comments.

@MrStevns MrStevns merged commit 623b585 into pencil2d:master Jul 25, 2019
@Jose-Moreno Jose-Moreno added this to the 0.6.5 milestone Jan 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants