garamond13 / w-image-viewer

Simple yet powerful image viewer.
MIT License
3 stars 0 forks source link

Just gave 1.0 a try and noticed a few issues #1

Open Jules-A opened 10 months ago

Jules-A commented 10 months ago

There were quite a few others I noticed but those were the main issues.

If you need me to supply extra info let me know.

garamond13 commented 10 months ago

Sometimes drag and drop results in checker-boarding black/white squares.

Don't think I understand, I would like more info on this.

Unable to maintain aspect ratio when resizing, I'm not even sure what window auto dimensions is supposed to do.

If you are talking about maintaining aspect ratio when resizing the window, that's is not implemented yet.

Window auto dimensions is auto sizing window when you open new image to dimensions of that image. If it would fit on the screen, and if not it still auto sizes but to max 90% of the screen resolution, while keeping the window aspect ratio same as the image aspect ratio.

There's no easy way to reset values to defaults

For now you can delete config file for this.

Zooming in too far with some scalers like FSR results in checker-boarding (falling back to bilinear instead would be better than showing nothing).

I would like more info on this.

Profiles are confusing, as numbers they make me assume it will do scaling within that range but it appears it's not the case.

With profiles you can set different scaling settings for different scale factor ranges. 0.0, 0.0 profile will be applied if no other profile covers current scale factor. Scaling factor of 1.0 will mean no scaling, less than 1.0 will be downscaling and larger than 1.0 will be upscaling.

No way to enable onscreen navigation for images.

Don't think I understand.

No way to separate scaling for Downscaling/Luma/Chroma ect.

Would have to separate luma and chroma since images will be decoded into RGB. Will look into this idea.

Downscaling you can handle with separate options using scale profiles, for an example 0.0, 1.0 will cover all downscaling.

No indication of the image's name or file path (I guess it could be added to window title to save space?)

You can change Window name setting to show you file name or full path.

There were quite a few others I noticed but those were the main issues.

I would like to hear about other issues as well.

Jules-A commented 10 months ago

Don't think I understand, I would like more info on this. It will look like this, like it failed to render. Though this specific image is when trying to zoom in with FSR scaling (no other option an auto dimensions set) but that's what it looks like.

image

For now you can delete config file for this.

I was talking about resetting a specific setting to default, deleting whole config isn't really ideal.

I would like more info on this.

Same as the first image, as soon as I try to zoom in it will break. Other scaling algorithms (blackman in this example) seem to artifact like: image

(the white dots)

EDIT: Actually it looks like it breaks when you don't have a profile that supports scaling to that zoom level. Instead of breaking when there's no profile to support that zoom level it should fall back to default or bilinear.

With profiles you can set different scaling settings for different scale factor ranges. 0.0, 0.0 profile will be applied if no other profile covers current scale factor. Scaling factor of 1.0 will mean no scaling, less than 1.0 will be downscaling and larger than 1.0 will be upscaling.

Ah I see now but it seemed to use the scaling methods even when zooming in higher than the specified profile amount.

Don't think I understand.

Look at Window's photos app, next and previous buttons show up when hovering close to the left/right side of the image.

You can change Window name setting to show you file name or full path.

Ah, it doesn't change on setting change but does on restart.

I would like to hear about other issues as well.

They aren't really issues so much as they are things I personally found annoying, mainly with defaults like opening such a tiny window (it should have some sort of scaling with Window res, ugly grey background, tiny settings window, when there's no image open a button to open image would be nice (or maybe use it to display recent image thumbnails) ect.

With profiles you are unable to change the min scaling above 0 until you change the max, it should just check when you try saving instead. Drag opening an image doesn't bring the app to the foreground. I'd also really like to be able to see the current zoom amount somehow. Opening an image moves the window to the center of the screen.

config.txt

garamond13 commented 10 months ago

About issues with Modified FSR and possibly other filters, I can reproduce. This is expected behavior due to wrong parameters. For an example: Modified FSR parameters have to be b != 0, b != 2, c != 0 (b - parameter1, c - parameter2). Probably should explain all filters in user manual (user manual is updated) and warn users when they try to set incorrect parameters.

..when there's no profile to support that zoom level it should fall back to default or bilinear.

It will fall back to the default profile 0.0, 0.0 and whatever settings are set for that profile.

Look at Window's photos app, next and previous buttons show up when hovering close to the left/right side of the image.

For now you can use Next and Previous from context menu or arrow keys. Also I can imagine this interfering with user wanting to drag image.

Ah, it doesn't change on setting change but does on restart.

It will be applied on the next opened image.

ugly grey background

Its neutral. That's just default, you can change it in general options to any color.

tiny settings window

Looks good on my end. Maybe is tiny due to high pixel density or something else. I would like to investigate this. Could you provide screenshot or take picture of you screen with camera?

when there's no image open a button to open image would be nice

For now you can open image from context menu or using shortcut CTRL+ O.

With profiles you are unable to change the min scaling above 0 until you change the max, it should just check when you try saving instead.

The problem with this is that it checks when you enter value is the range correct. Example upper bound (right box) is 0.0 and you wanna set lower bound (left box) first, on confirming value into lower bound it will do the check. But since you have to add profile to the list anyway I may rework this.

Drag opening an image doesn't bring the app to the foreground.

I'll consider this as an issue. Will look into fixing this.

I'd also really like to be able to see the current zoom amount somehow.

Will look into implementing this.

Opening an image moves the window to the center of the screen.

This is expected behavior, this will only be the case if you have Auto window dimensions enabled.

Jules-A commented 10 months ago

About issues with Modified FSR and possibly other filters, I can reproduce. This is expected behavior due to wrong parameters. For an example: Modified FSR parameters have to be b != 0, b != 2, c != 0 (b - parameter1, c - parameter2). ~Probably should explain all filters in user manual~ (user manual is updated) and warn users when they try to set incorrect parameters.

I just used default params, it probably should default to the correct params as not everyone knows/wants to look at a guide just to get scaling working.

It will fall back to the default profile 0.0, 0.0 and whatever settings are set for that profile.

It doesn't seem to be doing that or it is breaking somehow.

For now you can use Next and Previous from context menu or arrow keys. Also I can imagine this interfering with user wanting to drag image.

When it's just on the edges it doesn't really interfere with anything.

Looks good on my end. Maybe is tiny due to high pixel density or something else. I would like to investigate this. Could you provide screenshot or take picture of you screen with camera?

I'm just on 1440p 27" which is very standard these days but I'd imagine it'd be worse at 4k.

image image

By default the write changes just gets cut off for some tabs.

This is expected behavior, this will only be the case if you have Auto window dimensions enabled.

It's honestly rather annoying, is there a way to have it resize without the centering?

garamond13 commented 10 months ago

I just used default params, it probably should default to the correct params as not everyone knows/wants to look at a guide just to get scaling working.

Can agree with this, will look further into this.

When it's just on the edges it doesn't really interfere with anything.

Maybe it gets implemented after all.

It doesn't seem to be doing that or it is breaking somehow.

So far I couldn't find issue with this, it worked as expected.

Looking at those images its same as on my end. Are you referring to font size? Maybe should add option to use larger text and window.

By default the write changes just gets cut off for some tabs.

The settings window can be resized and it can be scrolled.

It's honestly rather annoying, is there a way to have it resize without the centering?

May implement option to enable or disable centering.

Jules-A commented 10 months ago

Looking at those images its same as on my end. Are you referring to font size? Maybe should add option to use larger text and window.

The settings window can be resized and it can be scrolled.

Sure but it's window sizes will reset on next launch....

garamond13 commented 10 months ago

Sure but it's window sizes will reset on next launch....

This should be addressed now among some other things in the new release v1.1.0.

Jules-A commented 10 months ago

This should be addressed now among some other things in the new release v1.1.0.

Just tried 1.2: Looks like the black checker-boarding was entirely scaling errors. After setting params properly it seems fine (though the user shouldn't have to do that for basic scaling). Params don't reset when changing scaler so it's easy to accidentally use wrong params. I don't understand the correct params for FSR. I ended up setting B=0.1, C=1 which seems fine but I don't get what B should be (anything over 0.5 looks terrible it seems).

It would be nice to be able to edit the profile scaling conditions, currently I have to add a new profile and delete the old one and set the scaling again which is a little tedious. The way scaling amounts are handled in profiles aren't nice either, I just realised I had 0.00000 - 1.000000 for downscaling but that meant it was using it with no zoom so I'd have to set the max to 0.999999 (I assumed my 1.00000 - 4.000000 profile would take precedence as the 4.000000 - 20.00000 one does at 4x zoom which is super inconsistent. I don't know if it's possible to see scaling higher than 6 digits creating some edge case which is why I used 1.000000 and not 0.999999.

The settings window remembering position doesn't seem to work too well if the app is maximized, settings window moved to the right-most then app is minimized. It's not completely hidden though but it doesn't retain the positions if it's maximized again (even if it didn't get moved).

The overlay is nice for debugging but wasn't really what I was after, something like Window's photo viewer which combines the titlebar with such information and quick settings would be nice but probably difficult, otherwise a small dedicated bar at the bottom (possibly also include mouse navigation) would probably be best but very subjective. A bar that disappears when there's no interaction for a some time could also be pretty cool (obviously if you are hovered over it already it shouldn't disappear).

Also noticed that zooming doesn't seem to take into account the position you are trying to zoom in (like Photo view does) and if you zoomed in and went to a corner of an image and then try zooming out it will end up showing no image as it will be off-screen.

Some wishlist requests would be ability to upscale (without zooming) then downscale (with a diff scaler) for the final output would be interesting. Being able to save the filtered (and possibly scaled) image would be awesome but out of scope for a viewer.

garamond13 commented 10 months ago

..(though the user shouldn't have to do that for basic scaling). Params don't reset when changing scaler so it's easy to accidentally use wrong params.

Don't see some perfect solution for this. Think we should simply set defaults instead of 0.0, 0.0 to 0.16, 1.0 cause this will be valid params for all functions.

I don't understand the correct params for FSR.

Can have look into my research, these params worked well in the research. For Modified FSR in general can tell you that C acts as a kernel blur.

It would be nice to be able to edit the profile scaling conditions

I agree with this, should look into implementing this.

..but that meant it was using it with no zoom

Its already optimized to skip any scaling if scale would be 1.0. https://github.com/garamond13/w-image-viewer/blob/d284294863caf122a06420b9072cca60fe9c3adf/w-image-viewer/renderer.cpp#L71C4-L71C33

I don't know if it's possible to see scaling higher than 6 digits

Scale is float so anything beyond 6 decimal places can be considered to be trash.

The settings window remembering position doesn't seem to work too well if the app is maximized, settings window moved to the right-most then app is minimized. It's not completely hidden though but it doesn't retain the positions if it's maximized again (even if it didn't get moved).

Don't know what do you mean by this. Did quick test of resizing settings window, moving it around all while maximizing application, minimizing, closing, reopening. All worked as expected.

The overlay is nice for debugging but wasn't really what I was after, something like Window's photo viewer which combines the titlebar with such information and quick settings would be nice but probably difficult, otherwise a small dedicated bar at the bottom (possibly also include mouse navigation) would probably be best but very subjective. A bar that disappears when there's no interaction for a some time could also be pretty cool (obviously if you are hovered over it already it shouldn't disappear).

So something like menu bar on top (under title bar) and like status bar bottom with which you can interact? W Image Viewer is imagined to be clean and simple, but maybe implement all that so that it can be turned on or off.

Also noticed that zooming doesn't seem to take into account the position you are trying to zoom in

It looks like its always zooming in center and not where your mouse cursor is. May look into changing this.

Some wishlist requests would be ability to upscale (without zooming) then downscale (with a diff scaler) for the final output would be interesting.

I don't see what good would this do.

Being able to save the filtered (and possibly scaled) image would be awesome but out of scope for a viewer.

I think this shouldn't be too difficult to handle.

Jules-A commented 10 months ago

Don't see some perfect solution for this. Think we should simply set defaults instead of 0.0, 0.0 to 0.16, 1.0 cause this will be valid params for all functions.

If you've already done tests for optimal params (which it looks like you have), can't you just use them?

Its already optimized to skip any scaling if scale would be 1.0.

Ah, well I'd still like to see scaling done at 1.0 but upscaling then downscaled back to 1x.

Don't know what do you mean by this. Did quick test of resizing settings window, moving it around all while maximizing application, minimizing, closing, reopening. All worked as expected.

https://github.com/garamond13/w-image-viewer/assets/1760158/a4f309ca-3307-4d98-9b15-1c4cb5872b22

So something like menu bar on top (under title bar) and like status bar bottom with which you can interact? W Image Viewer is imagined to be clean and simple, but maybe implement all that so that it can be turned on or off.

I was referring to using the title bar for such things like Windows Photo viewer to save viewing space but idk how to do that, might be hard. image

Having an intractable bar at the bottom wouldn't be too bad if it's automatically hidden after 400ms or so after not hovering near the bottom (it's how I have MPV set up so I'm used to it).

It looks like its always zooming in center and not where your mouse cursor is. May look into changing this.

I don't think it's always in the center as zooming out can cause the image to be completely off-screen.

I don't see what good would this do.

In subjective tests I've seen better results upscaling past native resolution and downscaling back to native with BC-Spline (b=0.3, c=0.35). So for a 1080p image, upscaling it something like 2x with FSR then downscaling to my native 1440p with BC-Spline (b=0.3, c=0.35) results in a better image than just simply upscaling from 1080p to 1440p. Although I did all my subjective tests with video in MPV so I'm not sure if that would translate to just images.

garamond13 commented 10 months ago

If you've already done tests for optimal params (which it looks like you have), can't you just use them?

Default params are already set to 0.16, 1.0 in the commit 305b26a.

Ah, well I'd still like to see scaling done at 1.0 but upscaling then downscaled back to 1x.

Any resampling will be destructive which means image quality will be objectively degraded compared to the original.

I was referring to using the title bar for such things like Windows Photo viewer to save viewing space but idk how to do that, might be hard.

Would have to switch to some custom title bar.

The issue with settings window shown in video is kind of expected behavior, even its not that nice.

Jules-A commented 10 months ago

Default params are already set to 0.16, 1.0 in the commit 305b26a.

But is that really a good solution for all scalers? Wouldn't it be better to use more optimal values as default for each?

Any resampling will be destructive which means image quality will be objectively degraded compared to the original.

I think maybe it depends on the original images quality, if it's poor quality it seems to benefit subjectively but it will deviate far from the original. I'm obviously not asking for it to be default but the option to do it would be nice.

Would have to switch to some custom title bar.

Hmm, I guess it would be rather tricky, for now would it be possible to have an option to remove the title bar with maybe a quick way to restore it somehow? For example: MPC-HC you can remove it all and re-add it in context menu: image

The issue with settings window shown in video is kind of expected behavior, even its not that nice.

Hmm... It's a tad annoying but not the end of the world. I guess you could save dimensions for 2 states separate for when window is minimized/maximized.

Also I found a small bug when maximizing the window then going fullscreen and exiting fullscreen, trying to minimize will do nothing.

Another feature request: Being able to easily swap between profiles from context menu, though not sure how to go about that? Stick with that profile until auto profiles are restored or change profile when the conditions for the profile other than the one you changed from are met?

garamond13 commented 10 months ago

But is that really a good solution for all scalers? Wouldn't it be better to use more optimal values as default for each?

No. But we can't have optimal params for all use cases anyway.

..would it be possible to have an option to remove the title bar with maybe a quick way to restore it somehow?

Not too optimistic about this, we need title bar to move the window. But maybe it gets implemented in some way.

Also I found a small bug when maximizing the window then going fullscreen and exiting fullscreen, trying to minimize will do nothing.

I tried to reproduce this. It all worked fine on my end.

Another feature request: Being able to easily swap between profiles from context menu, though not sure how to go about that? Stick with that profile until auto profiles are restored or change profile when the conditions for the profile other than the one you changed from are met?

So multiple profiles covering same range, and user chooses which one will be used. Maybe introduce custom names for profiles.

Jules-A commented 10 months ago

Not too optimistic about this, we need title bar to move the window. But maybe it gets implemented in some way.

I guess you could dedicate the top of the window for dragging the window around but not sure how difficult that is.

I tried to reproduce this. It all worked fine on my end.

Maybe a Windows10 issue? However no other apps seem to behave that way.

https://github.com/garamond13/w-image-viewer/assets/1760158/0c88539b-fe48-4f7d-ab00-341f4d510caa

So multiple profiles covering same range, and user chooses which one will be used. Maybe introduce custom names for profiles.

Actually I meant changing to a profile, completely ignoring ranges.

garamond13 commented 10 months ago

Maybe a Windows10 issue?

Don't thinks so. Think I see what you mean, will look into fixing this.

Actually I meant changing to a profile, completely ignoring ranges.

That can work too. Although wouldn't wanna overcomplicate things, its imagined to be simple to use and powerful image viewer.

Jules-A commented 10 months ago

Just found a newish bug, with center dimensions disabled it doesn't even try to fit the full image on screen or remember window positions:

https://github.com/garamond13/w-image-viewer/assets/1760158/353edc35-f0f7-4ba6-b9e1-7742f864ab1a

Also, I think being able to set minimum width/height is much needed, otherwise opening stuff like icons results in this: image

garamond13 commented 10 months ago

Just found a newish bug, with center dimensions disabled it doesn't even try to fit the full image on screen or remember window positions:

Should be fixed now.

Also, I think being able to set minimum width/height is much needed, otherwise opening stuff like icons results in this:

~Will look into fitting this inside settings.~ Implemented in the new version v1.3.0