-
Notifications
You must be signed in to change notification settings - Fork 58
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
New Metric: Altitude #168
base: master
Are you sure you want to change the base?
New Metric: Altitude #168
Conversation
backend-php/api/post.php
Outdated
$speed = isset($_POST["spd"]) ? floatval($_POST["spd"]) : null; | ||
$altitude = isset($_POST["alt"]) ? doubleval($_POST["alt"]) : null; | ||
$accuracy = isset($_POST["acc"]) ? floatval($_POST["acc"]) : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured out (correct me if I'm wrong) that converting String to float is not really necessary here, as it will end up in a JSON object anyway, therefore I got away with just one way of retrieving the metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion from String to float was that it ensures the value is always a number. In that case it is also saved to JSON as a float, and parsed directly by JavaScript into a float value, rather than a string. E.g. {"lat":50.2,"lon":20.1}
(float) vs {"lat":"50.2","lon":"20.1"}
(String).
backend-php/api/post.php
Outdated
// and a location list (l). Each entry in the location list contains a | ||
// latitude, longitude, timestamp, provider, accuracy and speed, in that | ||
// order, as an array. | ||
$session->addPoint([$lat, $lon, $time, $provider, $accuracy, $speed, $altitude])->save(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would make sense to store the values as a map, then it would be easier to retrieve them in the frontend (and also less error prone)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would definitely be an idea - storing the values as a map and saving it to storage as JSON would probably be a better approach than what I'm currently doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good, and a welcome addition to the project. I've proposed some changes that are mainly in how the markers are rendered. I suggest moving the altitude display to its own line underneath the velocity to make the marker label a bit more compact; see proposed changes.
Maybe it would be an idea to also allow altitude to be hidden? I.e. an altitude_unit
of NONE
that would hide it from the map altogether. From my experience, altitude fixes on phone GPS units can be.. inaccurate, and for most users, knowing user's altitudes probably isn't of great importance. Although for your use case, altitude display looks to be a very useful feature, and should definitely be included.
Any considerations on how to handle cases where a device only reports a 2D fix? Currently it appears to default to 0.0 meters, or the last available altitude, if any.
frontend/main.js
Outdated
'</span>' + | ||
'</p>' + | ||
'</div>', | ||
iconAnchor: [33, 18] | ||
// FIXME: hard-coded and dependend on style.css .marker | ||
iconAnchor: [60, 18] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iconAnchor: [60, 18] | |
iconAnchor: [37, 18] |
frontend/style.css
Outdated
@@ -254,7 +254,7 @@ a:last-child > .store-icon { | |||
|
|||
/* The outer marker div. */ | |||
.marker { | |||
width: 66px; | |||
width: 120px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing width from 66px to 74px to accommodate the extra rounding caused by moving altitude to its own line.
width: 120px; | |
width: 74px; |
frontend/main.js
Outdated
' | </span>' + | ||
'<span class="metric">' + | ||
'<span id="altitude-' + shares[user].id + '">0.0</span> ' + | ||
ALTITUDE_UNIT.unit + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding something that indicates that the metric shown is the altitude. Thoughts?
ALTITUDE_UNIT.unit + | |
ALTITUDE_UNIT.unit + ' alt.' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In aviation you would probably expect something like "250m AMSL", which indicates 250m above mean sea level. But that would probably look a bit odd below the speed:
12.5 km/h
250 m AMSL
I'll come up with something that looks good (hopefully), I just need to try out some ideas.
frontend/main.js
Outdated
' | </span>' + | ||
'<span class="metric">' + | ||
'<span id="altitude-' + shares[user].id + '">0.0</span> ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would probably feel a bit more compact if we add the altitude as a line underneath the velocity instead of on the same line.
Also suggest changing line-height
from 100% to 125% in .marker p
in main.css to make this two-line layout look better/more spaced out.
' | </span>' + | |
'<span class="metric">' + | |
'<span id="altitude-' + shares[user].id + '">0.0</span> ' + | |
'</span>' + | |
'<span class="metric">' + | |
'<br /><span id="altitude-' + shares[user].id + '">0.0</span> ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good idea. In fact I tried that first, but the alignment between speed and altitude looked awful. I need to brush up on my CSS skills a bit but I'll come up with something.
Thank you, I'm glad you like it :) I'll find a solution for your remarks and update the pull request.
I agree, that would look much nicer!
Supporting
If |
Regarding the metric panel, I played around on JSFiddle a bit. What do you think about this: Style 1Demo: https://jsfiddle.net/hqwjaxo8/2/ Style 2Demo: https://jsfiddle.net/hqwjaxo8/3/ Style 3I tested both styles in the frontend and it looks a bit dodgy. The monospaced labels look good in the demos, but in the frontend they look strange. Therefore I made it much simpler and now it looks fine to me. I did not include "AMSL", because there is no other altitude value, yet. |
@tuffnerdstuff thanks for this very good idea ! |
@2Belette I doubt that there is something like a 1-click option to compile an arbitrary Android Project. That said, it is much easier nowadays to set up your machine to do so than it was a few years ago. If you have any kind of Mac/Windows/Linux PC, just grab the Android SDK (command line tools are sufficient to compile), it's not too hard to set up. If you just want to try it out and/or you don't want to start hacking, then here is my locally built version. Use it on your own risk, though, it's always safer to compile stuff yourself. |
Sorry about being super late to get back to this. I've had a lot of other things on my hands and kind of forgot about this pull request. I'm not sure if we need both velocity and altitude to be explicitly labeled. My main concern was that altitude would be easy to mix up with distance (e.g. distance from map viewer) without labeling that specifies it as the altitude. I'm wondering if we could either stick to just e.g. "353.1 masl."/"1022 ft.amsl", or if we could maybe use icons in front of the labels as a replacement for text (which could also be more translation-friendly). Something like this is the concept I'm thinking of: 🚶 3.4 km/h I don't have time to make a mock-up right now, so I just used emoji for that demo, but I'm thinking we could use some kind of monochrome icons that fit well with the rest of the design. Let me know what you think, and I can write up a demo and post it this weekend. |
On my side I really like your idea! |
@bilde2910 would it be hard to maybe publish a beta version on F-Droid that we can test? I saw the concept of pre-release version which is not proposed to users but available when Unstables update option is ticked only |
No problem :)
Using icons was exactly what I was thinking too. I did a cursory search for suitable icons in the fontawesome set, but couldn't find anything. Maybe you're luckier than me :) I was thinking about some speedometer icon for "speed" and an arrow pointing upwards above wavy lines (water) for height AMSL. In that case we could also leave out the AMSL, as the icon would express the meaning. |
@bilde2910 just wondering if you plan to merge/integrate this nice feature soon into the F-droid app ? Or if something prevents it from happening ? many thanks! |
Using Hauk almost to all my trails and this enhancement would be very useful, I am wondering if the project is dead/in maintenance mode @bilde2910 ? it seems that what @tuffnerdstuff PR - work is 99% ready, is there anything can be done to be integrated in the addition of the Android App to make it happens ? Many thanks for the good job |
@2Belette if I recall correctly @bilde2910 wanted icons instead of text labels, so if you would like to help getting the feature merged, then feel free to propose a nice GUI solution. |
I've honestly been meaning to look into this pull request and the other issues on the repository for a while, but it's fallen completely off the radar, and for that I'm sorry. The truth is, I've been rather overwhelmed with commitments due to university and work, so I haven't been able to work on this project at all for a long time. I've been thinking of passing the main responsibility for the project to someone else, so that the project doesn't just die off. I haven't entirely made up my mind yet, but if anyone has any thoughts about this, I'd be happy to discuss. I'm open to granting co-contributor rights to the repository to someone else. |
@tuffnerdstuff @bilde2910 for this particular PR is it just a matter of icon vs. text which is blocking it or anything else ? If we want to stick with the current speed, text is fine but I think any of them is cool as long as we have the option it would be a very nice add-on. For the project globally, it would be indeed to bad if it would be abandoned as I really like it ! |
Is there any reason why this great proposal is still at the the same stage and blocked ? |
@bilde2910 would you consider to add this PR in the coming versions ? I am glad to see that the project is still live and your are back! So glad when I saw this morning F-Ddoid to propose the update |
Hi, I just came across your excellent project! As I want to use Hauk to keep track of my buddies when paragliding, I'd also like to have the altitude displayed. I made all necessary changes to the android app, backend and frontend (plus some refactoring) and hope you find it useful. I tested both unencrypted and encrypted communication and it looks fine. Let me know if something needs to be changed and feel free to cherry pick if you don't like my refactorings.
Cheers!
P.S.: If I find the time I would like to incorporate the OpenElevation API in order to get the height above ground and maybe also a custom icon for when the tracked user is airborne.