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

Guided value slider #11641

Merged
merged 14 commits into from
Jul 8, 2024
Merged

Guided value slider #11641

merged 14 commits into from
Jul 8, 2024

Conversation

DonLakeFlyer
Copy link
Contributor

Fixes #10743

See Issue for details

@DonLakeFlyer
Copy link
Contributor Author

DonLakeFlyer commented Jun 28, 2024

I'm lost. Creating a new pull didn't make it go away either.

@HTRamsey
Copy link
Collaborator

can you rebase locally and see what happens?

@DonLakeFlyer
Copy link
Contributor Author

DonLakeFlyer commented Jun 28, 2024

I already re-pulled the version you rebased tossing my old one. Trying to rebase that again just says it's up to date.

@DonLakeFlyer
Copy link
Contributor Author

Let me try making a whole new branch

@HTRamsey
Copy link
Collaborator

I would drop my rebase and do a merge just to see if one of the older commits didn't rebase well

@DonLakeFlyer
Copy link
Contributor Author

Stupid appveyor/ci#3585

@HTRamsey
Copy link
Collaborator

HTRamsey commented Jun 28, 2024

So I have no idea why the android builds fails to find gstreamer sometimes, but rerunning it works I think, I will look at it tonight. It might be related to the download from gstreamer's servers

@HTRamsey
Copy link
Collaborator

HTRamsey commented Jun 28, 2024

That's one of the things I will add to #11639
Edit: Nvm a quick google search gave me the answer. I can fix later today

@arnaudjuin
Copy link

That's one of the things I will add to #11639 Edit: Nvm a quick google search gave me the answer. I can fix later today

Any success or hint ?

@HTRamsey
Copy link
Collaborator

HTRamsey commented Jul 2, 2024

That's one of the things I will add to #11639 Edit: Nvm a quick google search gave me the answer. I can fix later today

Any success or hint ?

Sorry I've been working 15hrs the past few days and ran out of time. I will be able to do it tomorrow at the latest

@DonLakeFlyer
Copy link
Contributor Author

@JMare Can you retest?

  • Tick marks which are above/below allowed values are grey
  • Indicator is always centered
  • Fixed problem with setting below min alt

@JMare
Copy link
Contributor

JMare commented Jul 4, 2024 via email

* Various fixes to handle the fact that the slider height changes dynamically
* Ticks which are above/below allowed values have opacity set to 0.5
* Indicator is always centered in slider
@HTRamsey HTRamsey force-pushed the GuidedValueSlider branch from 5f99bbb to 3b133ff Compare July 4, 2024 13:17
@Davidsastresas
Copy link
Member

I added a few commits addressing some issues testing with Ardupilot.

The only one I think should be discussed is the last commit, about changing altitudeAMSL to altitudeRelative. Before this one, the slider would represent the altitude AMSL, even though the label says "alt (rel)" and it always represented relative altitude. @DonLakeFlyer was this a mistake or this brokes PX4 functionality? From what I saw in the code the issue explained above should affect PX4 as well, but I could have missed something.

Other than that I think it should be fine now.

The new slider looks amazing by the way, great job, thank you!

It comes as toFixed from Slider, so it should not be needed anyway
@DieBorr
Copy link
Contributor

DieBorr commented Jul 5, 2024

Hi, I have been taking a look to the slider, I really like it, seems really nice with the actual UI. However there is something that at least for me does not make sense and is that the horizontal flickable is visible when there is a width resize, I don't know if it has been made on purpose, I have a commit ready to push if you confirm that if it wasn't intentional. Correct me if I wrong but I can't figure out a case where the horizontal flickable needs to be visible in the slider.

image

PD: as I look the screenshot I think that we can also bring the altitude text steps to the left so the flickable does not overlap them.

@DonLakeFlyer
Copy link
Contributor Author

horizontal flickable is visible when there is a width resize

Ah, right that is screwed up. It isn't tracking contentWidth correctly on the flickable

@DieBorr
Copy link
Contributor

DieBorr commented Jul 7, 2024

#11663 solves overlap and makes some margin between text and slider border.

@DonLakeFlyer
Copy link
Contributor Author

I'm gonna merge this in and then fix the horizontal thing in another pull...

@DonLakeFlyer DonLakeFlyer merged commit 0205df9 into master Jul 8, 2024
9 checks passed
@DonLakeFlyer DonLakeFlyer deleted the GuidedValueSlider branch July 8, 2024 20:38
@JMare
Copy link
Contributor

JMare commented Jul 9, 2024

@DonLakeFlyer, Tested, and fixed the issue I saw previously - looks good.

The only thought I wanted to raise - which isn't specifically related to this change is:
When doing a change alt, QGroundControl sends the new altitude as an offset from the current altitude.
This offset is calculate by taking the new desired altitude and subtracting the current altitude.

One consequence of that is that if the vehicle is climbing or descending while you do the altitude change, there is an inaccuracy.
For instance, in Ardupilot SITL, if you do a changealt to 25.0m while you are climbing, you may find the vehicle actually settle at 25.5m. I can imagine this would be worse with high climb rates or low telemetry rates.

Its a minor thing but do you know if there is any reason the normal relative altitude couldn't be sent rather than an offset?

@DonLakeFlyer
Copy link
Contributor Author

Its a minor thing but do you know if there is any reason the normal relative altitude couldn't be sent rather than an offset?

If I remember correctly it's because PX4 firmware side does change alt as a delta offset. Let me look at the code again. Maybe better to send absolute all the way till it gets to mavlink and then adjust there. That way at least ArduPilot would get a good alt.

@DonLakeFlyer
Copy link
Contributor Author

Yeah, I looked at the code. I can reverse this so that an absolutely altitude flows through until the last minute instead of an offset. That way at least ArduPilot can get a solid alt change in the case described above.

@JMare
Copy link
Contributor

JMare commented Jul 10, 2024

Nice yes I think that would be a good change, happy to help with testing. (or coding, but it sounds like you have it covered)

@DonLakeFlyer
Copy link
Contributor Author

I had this backwards. It's ArduPilot that needs the offset change not PX4. Let me see if there is some different command I could send to get an absolute altitude change.

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.

Guided Altitude Slider Linear Slider with Snapping
6 participants