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

Fixed wrapping with Artificial Horizon #1324

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

Conversation

dronejunkie
Copy link
Contributor

This PR is to fixing an existing issue with artificial horizon wrapping in some situation. I did some regression testing as well with other single char elements like sidebar and stick overlay. No issue.

Copy link
Member

@mikeller mikeller left a comment

Choose a reason for hiding this comment

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

This does completely solve the problem, see

drag_drop_problem

I also think that trying to solve this problem by enforcing a specific origin point on the individual OSD elements with generated preview is not an ideal solution - this will require extra scrutiny to make sure that this constraint is not broken by any future changes.

I think in order to fix this in a robust way, what needs to be done is that the offset for the drag / drop attachement point that was introduced in #1165 and #1172 is taken into account when checking if the new position is clipping: Before #1165, OSD.searchLimitsElement() would always return the total extent of the element against the fixed attachement point for drag / drop, but now this needs to be relative to the variable attachement point.

@dronejunkie
Copy link
Contributor Author

dronejunkie commented Mar 25, 2019

I think we should able to use one logic to apply to all osd elements:string, array of char and array of strings.
String can be represented using array of string and its xMin, yMin is zero, xMax is the length of the longest element and yMax is the length of the array. Below can be the logic.

Proc OnDrop() {
    Let x and y be the co-ord for the osd element, which is the left top corner. 
    Let x' and y' be the mouse cursor co-ord. 
    x'= position MOD osd.width
    y'= position/osd.width
    x = x' + xmin - element.x
    y = y' + ymin - element.y
   
    if ( y < 0 ) {
        y = 0
    }
    if ( y + element.height > OSD.height )  {
        y = y - element.height 
    }
    if ( x < 0 ) {
        x = 0              
    } 
    if ( x + element.width ) > OSD.width ) {
        x = OSD.width - element.width
    } 

}

@mikeller
Copy link
Member

@dronejunkie: Yes, that sounds about right - what is neglected in the current code is proper calculation of the mouse cursor position relative to the element.

@dronejunkie
Copy link
Contributor Author

Sorry Michael, hopefully I will get sometime next week to completed this.

@stale
Copy link

stale bot commented Apr 30, 2019

This issue / pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs within a week.

@stale stale bot added the Inactive label Apr 30, 2019
@stale stale bot removed the Inactive label Apr 30, 2019
@mikeller mikeller added this to the 10.6.0 milestone Apr 30, 2019
@mikeller mikeller removed the Pinned label Apr 30, 2019
@mikeller mikeller modified the milestones: 10.6.0, 10.7.0 Sep 7, 2019
@mikeller mikeller modified the milestones: 10.7.0, 10.8.0 Apr 1, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs within a week.

@blckmn
Copy link
Member

blckmn commented Jun 18, 2021

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 -> FAIL
  • approver count at least three -> FAIL

@haslinghuis haslinghuis marked this pull request as draft June 19, 2021 00:25
@haslinghuis
Copy link
Member

Closing as inactive. Can be reopened when @dronejunkie decides to continue his work.

@haslinghuis haslinghuis closed this Jul 6, 2021
@mikeller
Copy link
Member

I'll reopen this - this is still a bug and needs to be fixed, and if somebody wants to spend some time on fixing this up it will be appreciated.

@mikeller mikeller reopened this Oct 10, 2021
@sonarcloud
Copy link

sonarcloud bot commented Oct 10, 2021

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell E 600 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@haslinghuis haslinghuis modified the milestones: 10.8.0, 10.8.1 Nov 23, 2021
@ctzsnooze
Copy link
Member

@dronejunkie - Hi Jack - any chance you can review this again? It would be great to fix this for 4.3.

@dronejunkie
Copy link
Contributor Author

dronejunkie commented Dec 5, 2021 via email

@haslinghuis haslinghuis modified the milestones: 10.8.1, 10.9.0 Jun 28, 2022
@haslinghuis haslinghuis moved this from Configurator to Firmware in Finalizing Firmware 4.4 Release Jul 12, 2022
@haslinghuis haslinghuis moved this from Firmware to Needs work in Finalizing Firmware 4.4 Release Jul 12, 2022
@haslinghuis haslinghuis moved this from Needs work to Inactive in Finalizing Firmware 4.4 Release Nov 13, 2022
@haslinghuis haslinghuis modified the milestones: 10.9.0, 10.10.0 Jan 29, 2023
@haslinghuis haslinghuis removed this from the 10.10.0 milestone Aug 3, 2023
@nerdCopter
Copy link
Member

is this still required, or is master working as expected now that HD was done? if still required, need rebase from master and continued testing.

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.

None yet

6 participants