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

Improve numbered image import #1235

Merged

Conversation

MrStevns
Copy link
Member

@MrStevns MrStevns commented Jul 6, 2019

What's new:

  • Added list with preview of the imported files, confirming which files will be added.
  • Error messages has been improved in case the user tries to import an image with wrong criteria.

image

image

The reason:
I noticed some inconsistencies and some things lacking using the new numbered image import, for example the import doesn't mention what it does, nor what you're doing wrong if you import the wrong file. It used a normal dialog instead of QImportExportDialog.

Missing info:
A new layer is created based on the prefix and the keyframes are added based on the image number but this is not mentioned anywhere.

Additionally I thought it would be nice to know what files are imported, when you select an image matching the criteria.

MrStevns added 2 commits Jul 6, 2019
This adds a preview window to make sure the user knows which images will be imported.
Additionally it also improves error messages in case the user tries to import an image with wrong criteria.
@davidlamhauge
Copy link
Contributor

davidlamhauge commented Jul 6, 2019

Well done @candyface !
This is a great improvement for the user.
I'm not sure why you delete the 'nameSuggest()' function. The purpose of this function is to suggest a layer name to the user, that is not already in use. Layer names should be diverse, unless the user specifically want equal layer names.

Another thing: This again shows how different the word 'intuitive' can be interpreted. If I spend the extra time on my files, to make sure they have the same prefix (e.g. 'boy') and numbered according to my timing of the scene (e.g. 'boy001.png', 'boy015.png, and so on), there could only be one purpose. I want them on a separate layer called 'boy', placed on the keyframes that corresponds to the three-digit number.
That's why I made the solution that is merged today. For me it is intuitive to pick a drawing from the series of drawings, and then expect the whole set of drawings to be imported on a separate layer.

I support your PR. This is intuitive and understandable for all.

@MrStevns
Copy link
Member Author

MrStevns commented Jul 6, 2019

@davidlamhauge I did not remove the nameSuggest function, I simply moved it and made it a core piece of the process of creating a new layer, instead of calling it in each function in actionsCommands. Take a look at LayerManager::nameSuggestLayer, we're still calling it, just not from ActionCommands :)

@Jose-Moreno
Copy link
Member

Jose-Moreno commented Jul 6, 2019

@candyface Thank you for doing this, good job 😄

On a more in depth discussion, I'm not quite sure the preview is a necessary addition for all use cases. One of the appeals of this import feature is that it is quick. Two or Three additional clicks are not really ideal if there's no important information or additional processing (e.g filters) you can do in that dialog window, when you can review the file criteria on your file explorer.

I mean conceptually it is a good idea to have a preview of the criteria, and for example in the image sequence import dialog we have it to a limited degree because you are gently forced to select which images should take part of the import.

However in the case of numbered images as I understand you are supposed to only pick one picture that defines the criteria and then Pencil2D will take care of the rest, in fact this is the best because you don't have to pick all the scattered images that are part of the sequence in case there are broken numbers in a large batch.

I'd think this "preview" step should be done in the file explorer too if possible... but with that said I'm also thinking this preview feature can be very useful in other scenarios, so from an objective point of view for this to be a complementary addition to the feature itself I think your proposal would also some additional tweaks:

  1. A scrollbar that appears when there are more images than the list container can show to actually review the different images
  2. The capability to select each image separately on the display (similar to David's pegbar alignment dialog where you can toggle select and shift-select all the images)
  3. A plus (add image) and minus (remove image) set of buttons to manipulate the selected images.
  4. A set of buttons to move the images up and down in their current list order.

Although let's please consider an alternative proposal:

Remove the image sequence numbered import dialog as a standalone option altogether, and merge its functionality with the image sequence import dialog. If we're talking about clarity and user experience, I don't see a reason to have two separate functions that do almost the same thing.

Let these functions be part of a general and more robust image sequence import dialog, and implement a toggle property called "one-click criteria", "keyframe drawing import" or similar where David's original behavior is toggled from the current image sequence import (and of course it can come with the appropriate explanation)

So if people want to see what they are selecting they uncheck this and proceed to select the images that fit the criteria... With this I would see even more value to include this preview into the merger dialog because it won't be working for one and not the other, but rather become an integrated part of the whole feature.

Please let me know what you think, because this is the perfect time to do that since you're working on these enhancements.

@davidlamhauge
Copy link
Contributor

davidlamhauge commented Jul 6, 2019

When I run your branch on a new file @candyface , and want to add a new bitmap layer, it suggests the same name as the existing bitmap layer, so something is not working. Hmm.
EDIT: Okay. I tested it once more. The problem is that the text in the small dialog states that te new name will be "Bitmap lag", but if you accept it in fact is "Bitmap lag 2".

@MrStevns
Copy link
Member Author

MrStevns commented Jul 6, 2019

When I run your branch on a new file @candyface , and want to add a new bitmap layer, it suggests the same name as the existing bitmap layer, so something is not working. Hmm.
EDIT: Okay. I tested it once more. The problem is that the text in the small dialog states that te new name will be "Bitmap lag", but if you accept it in fact is "Bitmap lag 2".

Ah I see.. yeah I've made a mistake, I'll fix it later. Thanks for pointing it out :)

@MrStevns
Copy link
Member Author

MrStevns commented Jul 6, 2019

A scrollbar that appears when there are more images than the list container can show to actually review the different images

There is a scrollbar already, you just can't see it because Mac OS hides it automatically until you scroll.

The capability to select each image separately on the display (similar to David's pegbar alignment dialog where you can toggle select and shift-select all the images)

I don't see the point of this, the keyframes will be imported based on the name and index of the files, changing the order of the preview window will not change anything. If one could change the order then there's no point to the rules which has been set for this importer.

A plus (add image) and minus (remove image) set of buttons to manipulate the selected images.
A set of buttons to move the images up and down in their current list order.

This could be an improvement to the normal image importer but I don't see why it would be useful here when the functionality is based on the filenames.

Remove the image sequence numbered import dialog as a standalone option altogether, and merge its functionality with the image sequence import dialog. If we're talking about clarity and user experience, I don't see a reason to have two separate functions that do almost the same thing.

They do not work the same way though, the image import sequence will import images based on a selection, it will add the keyframes on the current layer and the keyframes will be added from 0 and with a specified spacing.

The numbered image importer will only allow you to select one image in the explorer and then the rest will be added programatically. The process also adds a new layer based on the filename prefix eg. Joe if the file was Joe002.png, then all images that matches Joexxx.png will be added but they won't be added linearly, they are added based on the number, so Joe002 would create a keyframe on pos 2, Joe005 on pos 5 etc... on the timeline.

The image importers are fundamentally different in the way they work.

@Jose-Moreno I hear what you're saying but I don't think it fits the specific use case here, the idea here is not to change the way the numbered import works, merely help the user understand what they're importing. It's important to tell them what is imported, that's why the extra steps are necessary. Before unless you knew the functionality, you wouldn't know that it imports the keyframes in a certain way and that's no good, even if it's faster.

@Jose-Moreno
Copy link
Member

Jose-Moreno commented Jul 7, 2019

There is a scrollbar already, you just can't see it because Mac OS hides it automatically until you scroll.

That's great then 🙌

I don't see the point of this, the keyframes will be imported based on the name and index of the files, changing the order of the preview window will not change anything. If one could change the order then there's no point to the rules which has been set for this importer.

I agree that one would commonly either fix the order before import (in the file explorer) or after import (in Pencil2D) Although I meant this for the general purpose of re-ordering the files provided a file would be wrongly imported into the file order due to mismatched criteria or human error.

This could be an improvement to the normal image importer but I don't see why it would be useful here when the functionality is based on the filenames.

You mentioned that this PR is made to " ... help the user understand what they're importing."_ and I agree, but If we maintain the current import behavior, it means you can only select 1 file to load the images you want that comply with the naming criteria.

If the user made an unnoticed mistake while preparing the files and one or several don't meet the naming criteria how are they going to add or remove the files they need?

They'd have to import what they can, then go to File > Import > Image... and finish the job. If we're talking about making it easier to see what they are importing, I don't see why we can't also facilitate the possibility of importing what they missed or removing what wasn't part of the image sequence.

As you mentioned this idea could be a good thing to have in general, but I feel it becomes especially important in this "numbered" import feature because you can't cherry pick your files at all where there's a mistake. But to be fair, I guess we have to choose which approach has less shortcomings from a programming perspective and from a user perspective 🤔

It's important to tell them what is imported, that's why the extra steps are necessary. Before unless you knew the functionality, you wouldn't know that it imports the keyframes in a certain way and that's no good, even if it's faster.

Agreed and duly noted. Along that topic, we should also change the name of the function from Import Sequence Numbered to Import Key Drawings as I feel it represents better the intent of the function, and then the detailed explanation can finish conveying the functionality. Should I make a separate issue for this as a request?


The rest of my response was certainly unrelated to the PR itself, but I included it because i felt it was related to this feature. However I apologize as now I realize this could have been made a separate issue.

The image importers are fundamentally different in the way they work.

Yes, their implementation is different, but to an untrained user having two or more "similar" features is simply confusing. I don't see a reason to have two dialogs either, rather it'd be better for clarity to encapsulate everything related to "image sequences" in a single dialog and have both behaviors as alternate functionality, but I guess that's a discussion for another issue then ✌️

All in all, thank you for taking the time to prepare and submit this PR, as well as your replies 😄

@MrStevns
Copy link
Member Author

MrStevns commented Jul 7, 2019

agree that one would commonly either fix the order before import (in the file explorer) or after import (in Pencil2D) Although I meant this for the general purpose of re-ordering the files provided a file would be wrongly imported into the file order due to mismatched criteria or human error.

I mean I agree that it's easy to make human errors with the way the importer works right now, but if you could change the order of which frame would be where, then there would be no point in doing all that work beforehand. It's probably also unlikely that you know which frame is wrong until you import it all and do a playback test and by that time you might as well just use the "import image sequence" importer instead but I agree it's a flaw in the functionality.

I don't see why we can't also facilitate the possibility of importing what they missed or removing what wasn't part of the image sequence.

Because this import feature requires the filename to be written in a certain way to work as intended, otherwise you suddenly have files where the importer doesn't know where to put them on the timeline.

@MrStevns
Copy link
Member Author

MrStevns commented Jul 8, 2019

This will require more work but it could be done.

concept:
importdialog@2x

On the left you have a column with the imported images and on the right where the keyframe they will be placed. Importing a new image using the "+" button will either prompt to open a new dialog where you can write the keyframe or you'll have to write it in the right column.

You can reorder the frames using the arrow up and down buttons.

The layer dropdown will be filled with the name by default but the user can choose to select another.

Clicking on the eye will open a new window where the selected image will be shown. I thought about making the import of images instant (meaning right after you've closed the file explorer) and just show it on the canvas but then the window has to be a modal widget and not a dialog.

Being able to import a drawing at a time will require user action though, they can't simply select and import anymore because if you import a keyframe, you need to tell the importer where to put it on the timeline. This could be nuisance.

With this I'd still rather have two different dialogs for import a sequence and import a predefined set of keyframes than one dialog with tab complexity.

Although I still don't understand why the importer needs this kind of complexity. I'd imagine that you've scanned your drawings, so the files have been generated already with an incremental number, you've probably also looked at them before hand to make sure they're correct. Why would you make the process more cumbersome for yourself by doing it all in pencil2d?

@Jose-Moreno
Copy link
Member

Jose-Moreno commented Jul 8, 2019

@candyface That looks good. Ok, then how about we leave this PR alone, and migrate the additional enhancement discussion to a new issue.

Actually could we keep the dual column at least? I feel it's a bit clearer to show the path vs the frame it will occupy on the timeline. EDIT: If it's seen as unnecessary by the others then please dismiss it for this PR.

I take we're still missing a code review from any of the other devs, so when that's done I'm not opposed to merging this at all, in fact as I mentioned before, within the logic of giving context and clarity of intent for this feature back to the user, this is a good PR.

@davidlamhauge
Copy link
Contributor

davidlamhauge commented Jul 9, 2019

I think the first implementation you made @candyface is more than efficient.
If I scan some drawings and name them 'JohnDoe001.png', 'JohnDoe017.png' and so on, it must mine, and only mine, responsibility, that the prefix and numbers are correct.
I don't need the two columns. I know that the file 'JohnDoe017.png' will go to keyframe '017'. There is no need to tell me that.

@MrStevns
Copy link
Member Author

MrStevns commented Jul 9, 2019

I don't need the two columns. I know that the file 'JohnDoe017.png' will go to keyframe '017'. There is no need to tell me that.

@davidlamhauge But that's because you made it, you know the functionality inside out, what we need is verification from people with no knowledge about it :)

Actually could we keep the dual column at least? I feel it's a bit clearer to show the path vs the frame it will occupy on the timeline.

@Jose-Moreno I agree, I think it's better and the intent is clearer, adding that alone won't change the usability of the PR either so it should be a relatively minor change.

@MrStevns
Copy link
Member Author

MrStevns commented Jul 10, 2019

tableview

Ready for review:

  • Reworked preview
    • uses tableview now, with separated model for potential other uses.
  • Added progress bar.

@Jose-Moreno
Copy link
Member

Jose-Moreno commented Jul 10, 2019

@candyface Looking good! I forgot about mentioning the progress bar, glad to see you added that as well 😄

@davidlamhauge
Copy link
Contributor

davidlamhauge commented Aug 18, 2019

I have tested this branch thoroughly, and it works as expected in all cases.
I would have loved to review it as well, but there are simply too many things in the code I don't understand.
Since both I and @Jose-Moreno (and of course @candyface ) has tested this PR thoroughly, and the PR in many ways is an enhancement of an existing feature, I suggest that one reviewer would be enough, but that's not for me to decide. I can't review it :-(

Copy link
Contributor

@davidlamhauge davidlamhauge left a comment

We talked this PR through a couple of weeks ago, where you patiently explained the code I didn't understand.
I have now reviewed it again, and tested it, and I must say, that it look good to me.
Thanks for the effort!

@scribblemaniac scribblemaniac added this to the 0.6.5 milestone Sep 13, 2019
@scribblemaniac scribblemaniac added the 🔹 Minor PR (only one reviewer required) label Sep 13, 2019
@MrStevns MrStevns merged commit ace99a4 into pencil2d:master Sep 28, 2019
@MrStevns MrStevns deleted the Improve-refactor-importImage-numbered branch Sep 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement 🔹 Minor PR (only one reviewer required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants