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

#1177 Import drawings on wanted position #1285

Merged
merged 8 commits into from
Nov 9, 2019

Conversation

davidlamhauge
Copy link
Contributor

This addresses #1177, but I won't say it closes it.
There are 3 kinds oh image imports:
(1) image - (2) image sequence - (3) predefined set.
With this PR, you can select whether you you want to import (2) and (3) relative to:
(a) current view - (b) canvas centre - (c) current camera view.
The selection is done in a combobox on the filedialog.
You can't select where you want to import a single image (1), because we use the ordinary fileopen dialog in that case, and it's not possible to (for me at least) to place a combobox on that dialog. It could be solved by agreeing that all single image imports are done to canvas centre. Or - what do you think?
You can't select camera either, if you have multiple cameras. The importExportDialog doesn't know of layers or editors, and I'm not convinced that this possibility is necessary.

@MrStevns
Copy link
Member

MrStevns commented Oct 4, 2019

While I think that adding a combobox to each different import dialog is an okay solution that would work for some cases, it's limited because it's tied to a dialog and won't work for normal image import.

My own take on this would have been to import the images as you normally would and then add a post process step afterwards where you can drag the imported image around on the canvas, so you're 100% in control of where you want to import the image/s. There could be a floating window with a combobox of where you want to position for some easy adjustments but overall I think that the ability to position exactly where you want the image is crucial.

This would not be tied to a dialog and the same logic could work for importing a sequence of images too, because you would use the first image as pinpoint of where you want the other images.

My suggestion to make this PR work for all imports (disregarding my specific position comment above), would be to trigger another custom dialog specific for re-positioning after clicking "ok"/open on the import dialog, where you can select the combobox options you show in this PR.

This is better because:

  1. There's going to be less repetitive code
  2. It can be added to any future dialog, action or process.
  3. It will be easier to maintain.

@davidlamhauge
Copy link
Contributor Author

I had that in mind @candyface , but I thought it was somewhat clumsy. For the reasons you give, I can see it's in fact a better solution.
Your comments on a post process where you could reposition the drawings, reminds me of a feature I worked on half a year ago or something. I couldn't get it to work then, but maybe I got wiser since...? Not only for importing, but also for re-using animation, it's important that you can select a series of drawings and re-position them. That will be another PR.

@MrStevns MrStevns self-requested a review October 6, 2019 11:28
MrStevns and others added 4 commits October 8, 2019 18:58
* Decouple logic from ImportPositionDialog and the other import dialogs
* Rework PR logic for how image is re-positioned.
* Remember dialog setting.
Rework and refactor by CandyFace
@davidlamhauge
Copy link
Contributor Author

With good help from @candyface , I've made a new commit to this requested feature. I hope it fulfills the purpose.
You can now import images, image sequences, predefined image sets and gifs, relative to your view position or the camera view. Please feel free to test it.

@MrStevns MrStevns added 🔹 Minor PR (only one reviewer required) import-export labels Oct 12, 2019
Copy link
Member

@MrStevns MrStevns left a comment

Choose a reason for hiding this comment

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

Tested and works as expected.

A welcoming enhancement, although not fully ideal yet imo. ie. would be nice to have full control of where to position the image, it's still better than what we have.

Well done David 👍

@MrStevns MrStevns merged commit c8addc3 into pencil2d:master Nov 9, 2019
@chchwy chchwy added this to the 0.6.5 milestone Nov 10, 2019
@chchwy
Copy link
Member

chchwy commented Nov 10, 2019

Better to add to the milestone whenever merging a PR, it would be a lot easier for us in preparing the next release note.

@MrStevns
Copy link
Member

Yes of course, forgot about that Matt 👍

@scribblemaniac
Copy link
Member

I know I'm very late to the party here. I don't think that making another dialog was the best solution. It's quite annoying to have to go through 3 dialogs just to import an image sequence. Code duplication is a valid concern, but I think a more appropriate solution would have been to make a custom widget which could be added to each import dialog.

What happened to the center of currernt view option? I think that could be useful for situations where you want to import to a specific position, but don't want to modify the camera to do so.

Finally, importing with the "Center of active camera" option selected imports the images to the center of the camera at the first frame, but I would expect it to be centered on the camera at the time it is imported. Here's a demo of what I mean, because it is a bit difficult to describe:
Peek 2019-11-11 11-36
I would expect the imported images to remain in the center of the camera as it moves.

@MrStevns
Copy link
Member

MrStevns commented Nov 11, 2019

I agree that we could have make the flow less tedious by for example having a checkbox to avoid being asked every time, however I don't think having another widget in every import dialog is a better idea.

What happened to the center of currernt view option? I think that could be useful for situations where you want to import to a specific position, but don't want to modify the camera to do so.

Depends what you mean by view, what is "view" vs widget or canvas? 0,0? what is 0,0 in an infinite canvas?

Ideally I'd want the import to position to be much more visualized and not be bound to a dialog with the expectancy that one understand what either "center of canvas" or "center of active camera" or "view" without visually seeing what it means, so you would be able to freely move the image to where you want the image/s to be, with the addition of a floating widget that could be used to move the image to defined positions like similar to the implementation here.

As for the discovery you made, that's a bug it seems, I never considered testing an import where a camera transition ongoing :P

@scribblemaniac
Copy link
Member

I agree that we could have make the flow less tedious by for example having a checkbox to avoid being asked every time, however I don't think having another widget in every import dialog is a better idea.

To me that is saying that either you have to deal with the second dialog each time, or you you have to go to the preferences or wherever each time you want to change it. I don't know how often users will want to change this setting. If they want to change it often, it's better off in the same dialog as other options they might want to change (ex. frame spacing). And if they don't want to change it often, then it's better off being just a preference from the start. I think we should figure out what use cases are likely and choose the most appropriate option for it.

Ideally I'd want the import to position to be much more visualized and not be bound to a dialog with the expectancy that one understand what either "center of canvas" or "center of active camera" means, so you would be able to freely move the image to where you want the image to be, with the addition of a floating widget that could be used to move the image to defined positions like what it currently does.

Yes I do like this idea. Or we could allow it to be moved after importing similar to what's been proposed for a single frame except that moving the one frame affects all of them.

@MrStevns
Copy link
Member

Just a reminder: what was merged is not the final solution but it's one iteration that's usable and better than what was currently implemented. There's definitely room for improvement though.

I don't think options regarding import/export should be hidden in preferences, there are too many steps back and forth, it should definitely always be available, either through another widget in the import dialog or another dialog that shows later.

More buttons/settings in a dialog vs spreading it out through multiple tabs, windows or whatever is UI/UX and I'd say that it's better and easier to understand things where there are less of them, even if it requires more steps, however that's very subjective...

However none of these things matter if we rework the solution and allow moving the image/s in realtime :P

@Jose-Moreno
Copy link
Member

@scribblemaniac @candyface It's not a bug it's a feature! 😆 That's actually pretty cool, to import the files while the camera is moving! This can be used when you are animating a moving shot so you could load your layout or storyboard panels in order to roughly give it a sense of space, similar to this video here (notice the actual camera frames that are drawn have the effect of moving spatially, but all of it is 2D).

Watch the video by scrubbing frame by frame and you'll see what I mean regarding the camera framing:
https://www.sakugabooru.com/post/show/17755

I won't be opposed it it's "fixed", but just consider it 😉

Regarding the center of the canvas...I think we should at least consider the default canvas position with the camera frame middle point as the relative center of the "world".

image

In that sense the camera would then be moving in relation to that. This would be aligned with how actual traditional animators used to use the field chart, where the center of the screen was (0,0) and they moved North (-Y in CG) South(+Y) East (+X) & West (-X) to specific coordinates.
image

However related to this I recall you guys mentioned that the camera wasn't actually moving, but the canvas was. I guess this was meant originally to simulate a camera stand where the backgrounds were meant to be moved instead while taking the pictures (a la Disney), so if that's true, then it would still make sense that the camera was stationary while we better represent that what you're actually moving is the canvas itself? But if this is the case, then the center of the "world" should related to the camera, and not to the canvas 🤔

What do you think?

@davidlamhauge
Copy link
Contributor Author

I agree with @Jose-Moreno that it's not a bug but a feature. If you @scribblemaniac want your drawings to follow the camera movement, that should be another option in the menu/dropdown.
@candyface asks "what is 0,0 in an infinite canvas?". I think Jose explains and shows it in his comment. For 20+ years that fieldguide was good help for me in my professional career, and it's natural for an animator to think of 0,0 as the center.
I also agree that there is room for improvement. I worked on a feature some time ago, where you could move a series of drawings to wherever you wanted it. I couldn't make it work then, but maybe we should discuss (under another issue) how it should be implemented, the design of the dialog etc, so it could have another go?

@MrStevns
Copy link
Member

MrStevns commented Nov 13, 2019

@davidlamhauge, @Jose-Moreno I think you misunderstand what I mean

You have center of active camera
image

Then you have center of viewport (what I consider canvas... but viewport is not a layman thing)
image

This is fixed to the coordinate system of the application window

Then there's the coordinate system inside the unlimited drawing area, this has no height or width, no visual border, therefore no center.

What my point is that we could rephrase the options to something like:

  • Centre of active camera
  • Centre of canvas (some arbitrary point)
  • Centre of viewport (what I consider canvas)

if you guys think that's better.

@davidlamhauge
Copy link
Contributor Author

That's not how I see it @candyface .
The coordinate system inside the unlimited drawing area has indeed no height, width or visual border, but is has a center, and this center i (0,0).
I rely on it, @Jose-Moreno relies on it, and my old colleague Jakob Koch relies on it. He has told how he sometimes must re-import drawings, because they were not 'centered', and by centered he means according to (0,0).
Even the camera has a function called 'reset', that translates the camera to (0,0).
For me, and others, (0,0) is the absolute center of the infinite canvas, and thus a very important point.

@Jose-Moreno
Copy link
Member

@candyface Thank you for taking the time to clear up the definitions, I didn't know we had to consider the viewport as well like that.

As of 0.6.4 the images are being imported at the center of the "viewport". Unfortunately this is not consistent with how people experience import in other programs, and the reason behind # 1177

So the proposal was to choose between:

  • Active camera center: The averaged center of the camera point you obtain at (width / 2, height /2)
  • Paper canvas center: The "arbitrary" point on the surface that both the camera and the viewport are on top of when the zoom / rotate operator is used, or when a new file is created.

Having the center point either in the paper canvas or in the active camera center would initially fulfill the same role, to give a sense of virtual space for the artist.

I always think of it as If I had a drawing on my table and looked at it through a lens. I can move freely but the drawing will remain in the "same" place. The user is what moves virtually.

If I move the view I expect the images to be imported at the center of the camera / paper canvas always, because that represents my drawing area.

The caveat with having the origin at the center of the camera though, seems to be that if you "move" the camera you will create the nice effect Scribblemaniac showed. Moving the view should not change this though.

If we left the origin in the "paper" canvas however, moving the view or the camera should not effect the import location whatsoever. I can live with having the center origin at the center of the camera, but the "paper canvas" option seems to offer the most consistent behavior for me.

@chchwy
Copy link
Member

chchwy commented Nov 14, 2019

There is a center (0, 0) in the infinite canvas actually. It's the center of the world space.

The top-left point of a bitmap image sits in the world space. The image is transformed to camera space (with the camera position/rotation). and then is transformed to screen space (with the viewport) and finally be drawn on the window.

@MrStevns
Copy link
Member

@chchwy True, From a programming or matrix perspective there's a center but that's a technical knowledge that the user can't visually see.

Consider this example: You've created a preset or startup project where your camera always starts 1000 pixels to the left in the world space, would you still consider the point that is 1000 pixels (eg. the actual world space center) to the right to be the center and thus where to import the images, rather than where you currently are?

To me I'd expect the starting pos to be the center of canvas, since that's where I start drawing every time but maybe that's just me.. I still see center as an arbitrary coordinate in world space, even if that's technically 0,0 since there are no boundaries.

Whatever the case, let's just add the "0,0" as center of canvas again, your points are valid.

@scribblemaniac
Copy link
Member

The center of the canvas, from a user perspective can be viewed as the center of the viewport when the view is reset. Although I agree that it is quite arbitrary.

Anyway, I think all of this is missing the point I was trying to make. I'm not saying that having an import that is fixed relative to the canvas is a bad idea, but it absolutely should not be called "Center of active camera". That implies that well...the pictures will be in the center of the camera, and they're not right now! If you want to import a background that is fixed to the camera or something like that, this is a must.

Personally I think that an option to import to the center of the viewport is far more useful than the center of the canvas. First off you can import to the center of the canvas with it by simply resetting your view as users have to do in v0.6.4. Secondly, it allows you to fix the images to the canvas on any other location, not just 0,0, which allows for much more flexibility for the users to take advantage of than just a center of canvas option. I'm not opposed to having both of these options, but I am opposed to not having an option that follows the active camera.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔹 Minor PR (only one reviewer required)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants