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 centering of inner circle in PhotoVideoControl #12179

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rubenp02
Copy link
Contributor

@rubenp02 rubenp02 commented Dec 9, 2024

Fix centering of inner circle in PhotoVideoControl

Description

The inner circle in the PhotoVideoControl widget was not always perfectly centered due to a bug in Qt's anchors.centerIn property (QTBUG-95224) which causes it to return integer positions instead of subpixel values.

Implemented a workaround by manually calculating the center position, ensuring exact centering regardless of parent and child dimensions and DPI scaling.

This fix should be reverted once QGC moves to Qt 6.8.1 or higher, where the underlying Qt bug has been resolved.

Related Qt bug: https://bugreports.qt.io/browse/QTBUG-95224

Qt fix commit: qt/qtdeclarative@fd23a22

Checklist:

Related Issue

#10046

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@HTRamsey HTRamsey requested a review from DonLakeFlyer December 9, 2024 18:46
@rubenp02 rubenp02 force-pushed the bugfix/fix-record-button-alignment branch from bd9fd0f to 60a8187 Compare December 10, 2024 07:13
@DonLakeFlyer
Copy link
Contributor

due to a mismatch in the parity of the parent and child widths

Not quite sure what you mean by parity? Is this some sort of sub-pixel positioning problem?

@rubenp02
Copy link
Contributor Author

due to a mismatch in the parity of the parent and child widths

Not quite sure what you mean by parity? Is this some sort of sub-pixel positioning problem?

Parity as in being even or odd. I saw that the element positioning seemed integer-based, and if that's the case, an element with an odd width can't be exactly centered in one with an even width and vice versa. There's an error of 1 pixel per axis which then gets even worse with high DPI scaling. So the current adjustment rounds the child width to the same parity as the parent width.

However, your comment has made me realise that this problem is pretty weird considering that QML's positioning uses real numbers. So I experimented with it a bit more and found that the root cause is that anchors.centerIn always returns an integer position for some reason. Turns out this is a bug (QTBUG-95224) which has been fixed in Qt 6.8.1. So I'm going to adjust the fix so that it manually calculates the position correctly and uses the original, simpler logic for the width. This should be reverted once QGC moves to Qt 6.8.1 or higher.

The inner circle in the PhotoVideoControl widget was not always
perfectly centered due to a bug in Qt's anchors.centerIn property
(QTBUG-95224) which causes it to return integer positions instead of
subpixel values.

Implemented a workaround by manually calculating the center position,
ensuring exact centering regardless of parent and child dimensions and
DPI scaling.

This fix should be reverted once QGC moves to Qt 6.8.1 or higher, where
the underlying Qt bug has been resolved.

Related Qt bug: https://bugreports.qt.io/browse/QTBUG-95224

Qt fix commit:
qt/qtdeclarative@fd23a22
@rubenp02 rubenp02 force-pushed the bugfix/fix-record-button-alignment branch from 60a8187 to 7a88546 Compare December 11, 2024 11:12
@DonLakeFlyer
Copy link
Contributor

@HTRamsey What do you think the likelihood is that we should move to 6.8.1 before the next Stable? Is there anything holding that up?

@HTRamsey
Copy link
Collaborator

We can pretty much do it quickly today if you want. I think it builds as-is but there was a small change in linking QtMultimedia for Windows, the cmake translations command changes a bit, and I think we get a warning about the QSqlDatabase that needs a small fix. I had started to test Qt6.7 before doing Qt6.8 in #12145

@DonLakeFlyer
Copy link
Contributor

The problem is this centerIn bug is pretty major. This specific example here is just the tip of the iceberg with respect to QGC and use of centerIn with real number pixel positions.. That type of anchoring is used all over the place. Doesn't make sense to fix this one place when there is another 50 points of use. So I think moving to a new Qt would be worthwhile.

@HTRamsey
Copy link
Collaborator

@rubenp02 sounds like maybe we should just skip this and jump straight to 6.8.1. Does that sound alright in your opinion?

@rubenp02
Copy link
Contributor Author

@rubenp02 sounds like maybe we should just skip this and jump straight to 6.8.1. Does that sound alright in your opinion?

Yes, that makes sense if the move to Qt 6.8 is imminent

@rubenp02
Copy link
Contributor Author

Should I close this or are you still considering merging it?

@HTRamsey
Copy link
Collaborator

Since I'm going to switch the 6.8 real soon I would guess you can close it. Probably I will try to switch over during the next couple of weeks

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

Successfully merging this pull request may close these issues.

3 participants