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(fma): continuation of presel text fix #8328

Merged
merged 7 commits into from
Jan 7, 2024

Conversation

BravoMike99
Copy link
Contributor

Fixes #8217

Summary of Changes

This PR adds ":" to the SPEED SEL and MACH SEL text and slightly adjusts their position(continuation of #8230).

Screenshots (if necessary)

Left side is after while right side is before.
MACH SEL
SPEED SEL

References

Provided in #8230

Additional context

I had to map the "|" character to a bottom ":" character in the font file considering it is shared between multiple displays. Since the preselect ":" is aligned to the bottom, using the existing ":" would not work.

Discord username (if different from GitHub):
bruno_pt99

Testing instructions

Preselect a speed in the PERF CLB page (before takeoff). Make sure speed sel shows on the FMA with the ":" (if A/B is armed, it will only show after A/B message disappears).
Preselect a mach speed in the PERF CRZ page. Once climb phase is active make sure mach sel shows on the FMA with the ":".

How to download the PR for QA

Every new commit to this PR will cause a new A32NX artifact to be created, built, and uploaded.

  1. Make sure you are signed in to GitHub
  2. Click on the Checks tab on the PR
  3. On the left side, click on the bottom PR tab
  4. Click on the A32NX download link at the bottom of the page

Copy link
Member

@tracernz tracernz left a comment

Choose a reason for hiding this comment

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

Just a small formatting fix.

fbw-a32nx/src/systems/instruments/src/PFD/FMA.tsx Outdated Show resolved Hide resolved
@tracernz tracernz added this to the v0.12.0 milestone Dec 9, 2023
@lukecologne
Copy link
Member

lukecologne commented Dec 9, 2023

Hm, not sure if we need a new character in the font for this. From the refs for example here it looks like it could simply be a smaller font size compared to the rest? Also, is it strictly necessary to have the two different text fields? Seems unnecessarily complicated to me

@tracernz
Copy link
Member

tracernz commented Dec 10, 2023

The vertical alignment of the : relative to the baseline is very different to what's currently in the font.

@BravoMike99
Copy link
Contributor Author

BravoMike99 commented Dec 12, 2023

I made the split into two different text fields just to be able to manipulate the spacing between the presel text and preselected value since it was one of the suggestions of improvement in the other PR and seems to differ IRL between the MACH SEL and SPEED SEL. This is how the spacing looks on a single text element (ignore the : , used master branch).
MACH SEL2
SPEEDSEL

@lukecologne
Copy link
Member

Is the spacing really different between SPEED and MACH SEL? I assume you used the pictures from shomas from the original PR
(Pictures here side by side for convenience)

Header Header
271003339-ab61707b-7edd-4e58-bdce-ed2e86ce26d9 271796232-d25e193d-c6ec-4ec8-974f-ad6dd0921bee

The space in the MACH SEL is very obvious of course, but I'm pretty sure I see it in the SPEED SEL too (you have to compare either both the leftmost or rightmost edges of the blur).

You're right about the colon, it's weird that the alignment is so different... Although if you're splitting the text fields anyways it might be easier to just add a third text field just for the colon, and move that downwards, instead of doing a whole new character in the font

@BravoMike99
Copy link
Contributor Author

BravoMike99 commented Dec 12, 2023

Yeah I used shomas pics and maybe I wasn't clear enough, i meant the space/gap after the ":" which seems to be smaller in the SPEED SEL case. Regarding the colon, I can add a third element if that's preferable

@lukecologne
Copy link
Member

I don't know if that's preferable, just giving alternatives

BravoMike99 and others added 2 commits December 16, 2023 15:57
Co-authored-by: Michael Corcoran <tracer@outlook.co.nz>
@Popespice
Copy link

Popespice commented Jan 4, 2024

Quality Assurance Trainee Report

Discord : popespice
Object of testing: (#PR 8230/8328)
Tier of Testing : (1)
Date : (04/01/2024)

Testing Process:
Downloaded PR build
Planned short flight
Adjusted flight parameters in-flight (specifically speed and mach)
Observed aircraft and FMC behaved as expected,

Negatives:
N/A

Testing Results:
Passed

Conclusions:
Aircraft and system behaving as expected.

@Benjozork Benjozork enabled auto-merge (squash) January 7, 2024 13:20
@Benjozork Benjozork merged commit aebceb4 into flybywiresim:master Jan 7, 2024
6 checks passed
@2hwk 2hwk modified the milestones: v0.12.0, v0.11.2 Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✔️ Done
Development

Successfully merging this pull request may close these issues.

PFD SPEED SEL message
7 participants