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

Fix OSD preview #3101

Merged
merged 2 commits into from
Dec 5, 2022
Merged

Fix OSD preview #3101

merged 2 commits into from
Dec 5, 2022

Conversation

haslinghuis
Copy link
Member

Fixes: #3100

@haslinghuis haslinghuis added this to the 10.9.0 milestone Nov 29, 2022
@haslinghuis haslinghuis self-assigned this Nov 29, 2022
@github-actions

This comment has been minimized.

@blckmn
Copy link
Member

blckmn commented Nov 29, 2022

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> FAIL
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

@nerdCopter
Copy link
Member

nerdCopter commented Nov 29, 2022

hmm, well it does indeed fix the issue, but if the window width is less than ~1436 pixels, column 2 does not shrink enough and/or column 3 gets moved.

approx width 1436:
image

approx width 1432:
image

old behavior at approx width 1061:
image

width: 350px;
max-width: max(min(950px,calc((100vh - 470px) * 1.25)), 350px);
width: 740px;
max-width: max(min(950px,calc((100vh - 470px) * 1.25)), 740px);
Copy link
Member

Choose a reason for hiding this comment

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

The problem here is the "size" of the char that contains the symbol. It is 12x18 and we don't reduce it because it almost disappears because the reduction algorithm done by the browser works very bad with character images.
Until now, PAL and NTSC have 30 columns, so 30 x 12 = 360 px (the CSS rule was wrong by 10 pixels). Now, in HD it contains 53 columns, so the minimum must be 53 x 12 = 636.
This will make difficult to fit all without flexing, so the better approach is to give different sizes for both systems, at least PAL and NTSC will fit better. We have the code to add a class, and use different backgronds depending on the video system before this PR #3071 that removed it because it was not needed anymore.
So, two options:

  • Solution 1: simply use 636 as maximum
  • Solution 2: add the code removed in the PR to add a class based on the video system, and use 360 by default and 636 for HD.

Copy link
Member Author

@haslinghuis haslinghuis Nov 30, 2022

Choose a reason for hiding this comment

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

Thanks. Tried 636 but getting same behavior.

@nerdCopter
Copy link
Member

last commit 9f5e0e82 reverts to same behavior as bug and also limits minimal usable width.
this is approx 1332px at which anything smaller, then column 3 moves down.

image

@haslinghuis
Copy link
Member Author

Reverted for now.

@github-actions

This comment has been minimized.

@howels
Copy link

howels commented Dec 4, 2022

Could we arrange the Elements and Active OSD/Video Format columns below Preview on screens <1432?

Text moves outside the Preview frame currently:
image

@sonarcloud
Copy link

sonarcloud bot commented Dec 4, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link
Contributor

github-actions bot commented Dec 4, 2022

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
Betaflight-Configurator-Linux
Betaflight-Configurator-macOS
Betaflight-Configurator-Windows
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

@howels
Copy link

howels commented Dec 4, 2022

Looks to be working well, resizes the preview area and then moves columns underneath.

HD-preview-test.mp4

@haslinghuis haslinghuis merged commit 5cafebf into betaflight:master Dec 5, 2022
@haslinghuis haslinghuis deleted the fix-osd-preview branch December 5, 2022 11:19
@McGiverGim
Copy link
Member

It seems Android UI is broken now...

Screenshot_20221206-143950

@howels
Copy link

howels commented Dec 6, 2022

Android OSD preview isn't completely broken but the contents need to be aligned left and scaled.

Android-HD-OSD.mp4

@haslinghuis
Copy link
Member Author

On Samsung Galaxy S7 (SM-935F) with resolution 1440 x 2560 pixels the preview fills the screen (aligned left),
Only you can scroll to the right where there is white space,

What is the screen resolution of your device?

@McGiverGim
Copy link
Member

In my case, pixel 4 with 2280 × 1080.

@howels
Copy link

howels commented Dec 6, 2022

On Samsung Galaxy S7 (SM-935F) with resolution 1440 x 2560 pixels the preview fills the screen (aligned left), Only you can scroll to the right where there is white space,

What is the screen resolution of your device?

Sony Xperia 1 III, resolution 1644 x 3840. Guess it's down to the screen scaling. I am also running HD OSD.

@howels
Copy link

howels commented Dec 6, 2022

SD OSD is also offset but the text doesn't break the image preview box as much.
Screenshot_20221206-160822-01

@haslinghuis
Copy link
Member Author

haslinghuis commented Dec 6, 2022

Not sure how we are going to solve this for different resolutions as I have added a media query kicking in at 1455px or below and a, second at 1200px or below. They are based on FHD desktop screen (android does not have the menu visible as desktop does - this makes scaling even harder).

Maybe it would work better with the side panels dropped down below the preview as default on all platforms so we can use max instead of min. Happy to have some suggestions as I'm running out of idea's looking at the following table:

Device OS Resolution Status
FHD Desktop Ubuntu 22.04 1920 * 1080 OK
Sony Xperia 1 III Android 1644 x 3840
HP Elitebook Windows 10 Pro 1600 x 900 OK
Samsung Galaxy S7 Android 1440 * 2560 OK
Google Pixel 4 Stock Android 1080 * 2280
Nokia G60 Stock Android 1080 * 2408 N/A

@howels
Copy link

howels commented Dec 6, 2022

Could we scale the preview window on mobile? I know on desktop we want to avoid this but on mobile the touch targets are already challenging anyway. Losing fidelity in the fonts isn't a big deal on very small screens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

master - OSD tab - HD grid scale/z-level issue
6 participants