More intuitive work with camera movements. Camera rewrite (2)

[davidlamhauge]

This PR is premature, but it seemed to me like the best way to talk and discuss the development of the camera.
The branch is based on the branch that is under review right now. I think that big changes should come gradually.
The greatest change is that you can zoom on the camera layer, without changing your camera-field. You do that by dragging and scaling the field freely across the background. There is also a preview of the camera path...
I'm looking forward to discuss this.
closes #766
https://youtu.be/fkMOJ1c93r0

[davidlamhauge]

Just a small update on the new camera, and how I would like it to be. NB: This video is not made with the branch in this PR, but I intend to merge it into this branch, when I'm done testing it. The branch used for this video is called 'camera_rotation'
https://youtu.be/YlOkEes3wtM
Edit: The branch camera_rotation is now merged into the PR branch, and the video reflects the new features.

[MrStevns]

Alright i'll stop now... I keep finding things I want to improve and I really can't help it when i see it crash somewhere which may or may not be related to this... but i'll stop now 🙈 .

@J5lx please review again, i've addressed all points but those that I asked about previously.

There are once again quite a lot of changes but the majority of the changes are related to either bugs or what you mentioned, the rest are stuff that I noticed were missing or forgot to handle previously or new stuff that I felt was needed to make the implementation more stable.

To sum it up:

• Camera is painted properly when using relative, single mode
• Fixed issues surrounding onion skinning
• Updated camera reset icons
• Fixed various crash situations, mostly surrounding removal of keyframe
• Fixed camera properties were not updated properly on project load
• Color of handles are now the same as the select tool (ie. not based on palette anymore)
• Prevent deletion of (in UI and core) a camera layer if there's only one
• Cursor now rotate based on the camera orientation (for camera tool only)
• Improved performance regarding cache and painting
• Fix handles not being properly updated when playback stops
• Various refactoring and cleanup...
• Disable camera context menu actions when applicable
• Camera rotation handle is now shown above the camera and also replaces the triangle
• Camera handles are always shown now but painted as hollow when they can't be used

[Jose-Moreno]

@MrStevns Don't worry I'm sure more things will surface but It's better that we address them in separate PR's to keep track of them, and that way other contributors can also assist to implement the fixes in due time.

If you found other crashes please report them separately as you have before. I'll also gladly test these improvements little by little and if I find crashes I'll be sure to report as well or add to the ones you submit.

So far everyone has done an outstanding job, and I'm really hopeful for the immediate future of the software 🙂 🙏

Thanks lots to @davidlamhauge and @J5lx for pulling through the implementation and the reviews as well!

[J5lx]

Thanks for all your work @MrStevns! The PR is already shaping up quite nicely. I only have a few remarks on the code and a little bit of cleanup that I’ll push in a moment. Other than that, the changes you made all look good to me. I’m pretty excited!

Edit – one thing I forgot: the translation reset icon looks quite busy in the UI (at least on my normal DPI screen), I feel like it would look clearer without the little corners (i. e. only the arrows and the rectangle).

Just to make sure it doesn’t get lost among all the other comments, here are the only two remarks from my original review that still stand:

• Just a thought: Rather than updating the path control point whenever one of the “surrounding” keyframes is moved, perhaps it would be simpler to leave it at some sort of “null value” and fall back to a lower order Bézier curve (i.e. linear) when calculating the interpolated values in that case
• Why is antialiasing enabled for overlays but not for the camera?

And now to address your comments…

wouldn't I have to iterate through the existing xml and check if the property exists first

No, you would basically just do if (cam.controlPointMoved) { /* write pathCPX and pathCPY */ } /* else do nothing */. And on file load, the presence of these attributes would then imply controlPointMoved = true. Unless I’m overlooking something.

Could you elaborate on this

Basically, (1) start dragging the camera (2) while dragging, use the . and , to move to a different frame and back (3) stop dragging. (You could also stop dragging before moving back to the original frame). Now try dragging the camera again – it won’t move. But if you try again after you stop pressing the mouse button, it works again. I can reproduce this quite consistently.

I have seen none that include reset buttons, so I'm not sure this argument hold.

I’m not sure what difference that makes. The previous designs didn’t exactly scream “reset” at me either.

The icons are supposed to make you think of the camera itself

That’s interesting. Now that you mention this I can kinda see what you were going for originally. I think the new icons you came up with are a good compromise retaining that original idea while communicating the purpose more clearly. Well done!

Cursor now rotate based on the camera orientation

That’s a neat little detail!

Camera handles are always shown now but painted as hollow when they can't be used

Should apply this to the path control point handle too.

[MrStevns]

Basically, (1) start dragging the camera (2) while dragging, use the . and , to move to a different frame and back (3) stop dragging. (You could also stop dragging before moving back to the original frame). Now try dragging the camera again – it won’t move. But if you try again after you stop pressing the mouse button, it works again. I can reproduce this quite consistently.

Ah... like that! I see, yeah I can reproduce that behavior.
Aside from the cursor being wrong when pressing again, what were you expecting? you can't move the camera unless you're on a keyframe, so to me the behavior works as expected or do you mean that we should allow this and keep track of the frame while pressed? The issue with applying such a change right now is that the handles will look wrong even though you can drag because the painter logic is decoupled from the tool. I'd like to address this in the future but let's call it a "known issue" for now.

Should apply this to the path control point handle too.

Already done in ba42385

I’m not sure what difference that makes. The previous designs didn’t exactly scream “reset” at me either.

That's because they were not meant to, well not on their own anyway :P the label is meant to convey the action and the icons the context of what we reset. :)

Edit – one thing I forgot: the translation reset icon looks quite busy in the UI (at least on my normal DPI screen), I feel like it would look clearer without the little corners (i. e. only the arrows and the rectangle).

This gave me an excuse to cleanup the svg data some more. Additionally i've replaced the png cursor icons with svg.
The translation reset icon now looks like this:

Some more things I think should be done after this PR:

• Create stronger coupling between the camera painter and the camera tool.. to avoid doing duplicate calculations...
• Create a base cache functionality for all our painters that are related to the canvas, it's tiring and annoying having to create this each time.
• Allow dragging a camera even though you've scrubbed, this will become easier when the first point has been addressed.
• Fix DPI ratio being wrong for cache, making the canvas looks lower res while a tool is active.

[J5lx]

I added another change to make the pathCPM attribute implicit, which also fixes an issue when loading projects saved without this PR. Everything looks good to me now!

what were you expecting?

The main issue that I saw is that the way that the camera rect would just not respond to dragging at all until you click once. But perhaps you’re right and we should just keep it as a known issue for now…

Already done in ba42385

I could have sworn it didn’t work the last time I tried, but you’re right, it’s already done…

The translation reset icon now looks like this

Yup, that’s much better!

Some more things I think should be done after this PR

Agreed.

[J5lx]

I’d say this PR has seen a sufficient amount of review now. If any other issues remain, we’ll fix them whenever they become known.