search

HaikuArchives / ffmpegGUI

GUI for FFmpeg
MIT License
23 stars 10 forks source link

Improved cropping layout #116

Closed humdingerb closed 1 year ago

humdingerb commented 1 year ago

Cropping parameters and preview arrow buttons to the left, cropping view on the right.

humdingerb commented 1 year ago

It would be nice to have the CropView left-aligned, snug to the cropping parameters. especially for tik-tok-scape (=portrait) aspect. Unfortuntely, I haven't found a way yet to make it left-aligned without it not also limiting its height. A simple AddGlue() to its right doesn't cut it. Also playing with the view's and glue's weight wasn't successful... I suppose good enough for a first stab though...

andimachovec commented 1 year ago

It would be nice to have the CropView left-aligned, snug to the cropping parameters. especially for tik-tok-scape (=portrait) aspect. Unfortuntely, I haven't found a way yet to make it left-aligned without it not also limiting its height. A simple AddGlue() to its right doesn't cut it. Also playing with the view's and glue's weight wasn't successful...

Are you sure the problem is with the size of the actual CropView, not with the size of the image itself?. There could of course be bugs and limitiations in my image sizing code within CropView. If in doubt you can add a StrokeRectangle(Bounds()) statement with the in CropView::Draw() for testing purposes to visualize the actual size of the CropView.

I suppose good enough for a first stab though...

Doesn't look too bad at all, definitely a big improvement. One small suggestion: you could replace the "Preview images" text between the left and right buttons with the index number of the current image and the total number of images, eg. "1/5". This should make implicitely clear what the buttons are there for. The values are in fCurrentCropImageIndex (+1 needed) and fCropImageCount respectively.

humdingerb commented 1 year ago

Are you sure the problem is with the size of the actual CropView, not with the size of the image itself?

Well, it's an issue with the layout of the "croppingoptionsview". You have the group of cropping parameters to the left and the preview to the right of it. The parameters stay nicely fixed at the left, but the preview centers in the rest of the available space. I'd like to see how it looks if it were left-aligned in the available space, but adding AddGlue() to its right doesn't work as intended. It just grabs space for itself, stealing it from the preview. It's difficult to explain. Feel free to experiment with it... :)

One small suggestion: you could replace the "Preview images" ...

Actually. I found that the current preview system doesn't work well for long videos. It takes forever and creates tons on preview images. I experimented a bit and found a way that's possibly way better: Grab just one random frame and add a "Random preview" button to load another if the current one doesn't suit. What do you say?

humdingerb commented 1 year ago

I couldn't stop myself and implemented the randomized preview. PR is updated, check it out. :)

andimachovec commented 1 year ago

Actually. I found that the current preview system doesn't work well for long videos. It takes forever and creates tons on preview images

I wasn't that fond of the currently used ffmpeg command to generate the preview images either. It was only meant as a stopgap solution until we find something better.

Grab just one random frame and add a "Random preview" button to load another if the current one doesn't suit. What do you say?

Great, let's use that.

I couldn't stop myself and implemented the randomized preview. PR is updated, check it out. :)

Works very well, I like it. Not completely sure about the placement of the "New Preview" button. I can't really explain why but I'd expect it to be down at the bottom below the reset button. ;-)

humdingerb commented 1 year ago

Not completely sure about the placement of the "New Preview" button. I can't really explain why but I'd expect it to be down at the bottom below the reset button. ;-)

Funnily enough, I had it down below the "Reset" button, but it didn't look that nice.

How about we merge and see if we can make any marginal improvements later, if possible?

andimachovec commented 1 year ago

How about we merge and see if we can make any marginal improvements later, if possible?

Yep. Sounds good :+1: