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

Improved compass arrow #4689

Merged
merged 2 commits into from
Sep 17, 2024
Merged

Improved compass arrow #4689

merged 2 commits into from
Sep 17, 2024

Conversation

Szetya
Copy link
Contributor

@Szetya Szetya commented Sep 12, 2024

#4494
New compass arrow and replacement of the north marker with a small circle.

@CLAassistant
Copy link

CLAassistant commented Sep 12, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@HarukiToreda
Copy link
Contributor

Very nice! I works really well on the T114
20240914_020915

@Szetya
Copy link
Contributor Author

Szetya commented Sep 14, 2024

Something went wrong. The pull did not go through. @thebentern can you help?
The old compass variables left in as a comment.

@todd-herbert
Copy link
Contributor

todd-herbert commented Sep 14, 2024

It's definitely always bugged me too that the N is off the top of the display!
Do you feel that meaning of the small circle as a north indicator is clear enough?
The solid fill on the arrow might cause ghosting on some E-Ink displays; it's a lot harder to clear solid blocks of color like that.


Something went wrong. The pull did not go through.

image

Are you referring to this? That was just your changes being updated, because the firmware repo moved forward while this PR is still pending.

image

Are you referring to this? This means the layout of your code (spaces, tabs, etc) doesn't match the rules. Normally people fix this automatically with the https://trunk.io/ tool, but someone can probably help fix it for you this time.

@Szetya
Copy link
Contributor Author

Szetya commented Sep 14, 2024

It's definitely always bugged me too that the N is off the top of the display! Do you feel that meaning of the small circle as a north indicator is clear enough? The solid fill on the arrow might cause ghosting on some E-Ink displays; it's a lot harder to clear solid blocks of color like that.

Something went wrong. The pull did not go through.

image

Are you referring to this? That was just your changes being updated, because the firmware repo moved forward while this PR is still pending.

image

Are you referring to this? This means the layout of your code (spaces, tabs, etc) doesn't match the rules. Normally people fix this automatically with the https://trunk.io/ tool, but someone can probably help fix it for you this time.

Hi Todd!
Thanks for your reply!
Yes, I think the small circle is clear. I have looked at 4pixel radius as a good size for all displays as I tested (Heltec VM290, T-echo, SSD1306, Heltec T114, Heltec Wireless Tracker)
On E-ink displays, the ghosting was not distracting to me. But if you see fit for a directive e-ink case fillTriangle instead of drawTriangle could work.
I'm not an experienced github user, but the arrow and N... was confusing and too simple. :) Obviously in the beginning for oled display was just enough.
Yes it is likely to be a bug but I only edited it on the web interface. Could be a different tab size or more tabs before the comment (I like to drag the comment to the side) but that could be a rule.

@todd-herbert
Copy link
Contributor

todd-herbert commented Sep 14, 2024

On E-ink displays, the ghosting was not distracting to me. But if you see fit for a directive e-ink case fillTriangle instead of drawTriangle could work.

Some display models do have more issues with ghosting that others, but probably it's more concerning from a technical standpoint than a user-experience standpoint.

But if you see fit for a directive e-ink case fillTriangle instead of drawTriangle could work.

That's maybe not a bad idea actually. At least it'd be a relatively small block of code to wrap. I know I've committed greater preprocessor sins myself previously when dodging filled black elements for E-Ink.

but the arrow and N... was confusing and too simple

As for confusing: I've always had "Compass north top" set, although that's personal taste.
As for too simple: I'm slowly working away at some UI changes for E-Ink displays which hopefully will be appealing when they're ready.

Yes it is likely to be a bug but I only edited it on the web interface. Could be a different tab size or more tabs before the comment (I like to drag the comment to the side) but that could be a rule.

Ah don't worry, it's a bit tricky to wrangle the formatting: it's fussy. I couldn't do it manually. We all just rely on linting tools like trunk.

@Szetya
Copy link
Contributor Author

Szetya commented Sep 14, 2024

On E-ink displays, the ghosting was not distracting to me. But if you see fit for a directive e-ink case fillTriangle instead of drawTriangle could work.

Some display models do have more issues with ghosting that others, but probably it's more concerning from a technical standpoint than a user-experience standpoint.

But if you see fit for a directive e-ink case fillTriangle instead of drawTriangle could work.

That's maybe not a bad idea actually. At least it'd be a relatively small block of code to wrap. I know I've committed greater preprocessor sins myself previously when dodging filled black elements for E-Ink.
I was thinking something like.
#ifdef EINK_CS
display->drawTriangle(tip.x, tip.y, rightArrow.x, rightArrow.y, tail.x, tail.y);
#else
display->fillTriangle(tip.x, tip.y, rightArrow.x, rightArrow.y, tail.x, tail.y);
#endif
display->drawTriangle(tip.x, tip.y, leftArrow.x, leftArrow.y, tail.x, tail.y);
But there can be an empty arrow for each display. :)

but the arrow and N... was confusing and too simple

As for confusing: I've always had "Compass north top" set, although that's personal taste. As for too simple: I'm slowly working away at some UI changes for E-Ink displays which hopefully will be appealing when they're ready.
Also. And it's not really turom to interpret if it's in heading mode. :)
The circle is always regular and visible. Even if the compass is not set to "north top".

Yes it is likely to be a bug but I only edited it on the web interface. Could be a different tab size or more tabs before the comment (I like to drag the comment to the side) but that could be a rule.

Ah don't worry, it's a bit tricky to wrangle the formatting: it's fussy. I couldn't do it manually. We all just rely on linting tools like trunk.
I'll be honest but I don't want to put any more effort into it because I don't think I'll be able to add anything worthwhile to the code in the future. :)

@todd-herbert
Copy link
Contributor

I wanted to run trunk to fix the formatting for you and make GitHub happy, but I threw in that directive at the same time. Sorry for the intrusion.


Personally I'm still not sure how clear the meaning of the indicator circle is without the N, but if nobody else is worried by this, I'm happy to let it go.

@todd-herbert
Copy link
Contributor

todd-herbert commented Sep 14, 2024

I don't think I'll be able to add anything worthwhile to the code in the future. :)

There's always things to fix if you're up for it!

@Szetya
Copy link
Contributor Author

Szetya commented Sep 14, 2024

@todd-herbert
I see the formatting is strict. Understandable, of course, when so many people are working on it.
Oh, really the USE_EINK macro is much more suitable. Perfect!
Thanks for the help and professional advice on the filled shapes!
This is not an intrusion. Here's nothing to apologise for! In fact, thank you very much!
At the moment, the points on the N mark are used as comments in case users don't like the circle.
I have tried several solutions: Small triangle within the circle of the compass, N on the circle, short line on the circle. This was probably the best. If anyone has a better idea... We will implement it, we will replace it. :)

@fifieldt
Copy link
Contributor

Thank you for this work.

@todd-herbert I think the circle is OK! :) It probably makes more sense in places that don't use an English alphabet, too!

@Szetya
Copy link
Contributor Author

Szetya commented Sep 16, 2024

Thank you for this work.

@todd-herbert I think the circle is OK! :) It probably makes more sense in places that don't use an English alphabet, too!

I thank you all for your help and support!
I have used it for a long time ago. But each time I have corrected it manually. 😇 I didn't like the shape of the arrow first of all, secondly the N symbol sticking out of the screen. Especially on TFT displays where the resolution would allow a more serious design. I know how the UI is made and very nice style. But until then...
Related open requests:
#4688
#4494

A very amateur question.Is there anything else I need to do regarding the pull?

Szetya and others added 2 commits September 17, 2024 08:27
meshtastic#4494
New compass arrow and replacement of the north marker with a small circle.
@fifieldt fifieldt changed the title Compass update Improved compass arrow Sep 17, 2024
@fifieldt
Copy link
Contributor

@Szetya nothing left for you to do - we're just waiting for those checks to run then we can hit the merge button.

@fifieldt fifieldt merged commit a511878 into meshtastic:master Sep 17, 2024
106 checks passed
@todd-herbert
Copy link
Contributor

@Szetya passing this on from the Discord server
appreciative user comments

@Szetya
Copy link
Contributor Author

Szetya commented Sep 23, 2024

@Szetya passing this on from the Discord server appreciative user comments

Thank you for your kind feedback!
I don't use Discord. It was nice to see the satisfaction. :)
I've rethought the whole thing a bit. Mostly because in "north top" mode there is no feedback on the direction of travel.
The small circle is the north direction indicator. (A substitute for the letter "N".)
The small line is the direction of travel. (That's new.)
The big arrow is the node direction. (As before.)
-In north top mode, the small circle is fixed at the top. The short line indicates the direction of movement relative to the north.
In this case, you can see which way you are moving relative to the north and the node (if the big arrow is pointing to the small line, you are moving in a straight line in the direction of the node)
Picture of the north top mode:
image

-When north top mode is off. The short line is fixed at the top. You can see where north is relative to the direction of movement as well as the direction of the node.
Picture of heading top mode:
image

I tried to test it as much as I could. The nodes I know and the north as well as the direction of travel seem to have been correct during the tests.
(the heading caption and angle display just debug for me)
Reasons for choosing the short line:
-for low resolution displays e.g. OLED it is a clear indication.
-when intersecting the small circle it is still visible and clear that we are moving north.
ps.: I don't want to force it on anyone. It's just my "crazy stupidity". :)

@fifieldt
Copy link
Contributor

Looks great @Szetya ! Want to try proposing another patch :D

@gala-vine
Copy link

This is a good idea,, looks good should certainly be implemented as an option. my only issue with the compass it that at its basic usage it should work as such, like when i open the compass app on my phone and it points to north, or even if i had a real compass for that matter!!. i'd like it to point to north and the arrow point towards where the node is.. so maybe they could be the new updated options..

@Szetya
Copy link
Contributor Author

Szetya commented Sep 24, 2024

This is a good idea,, looks good should certainly be implemented as an option. my only issue with the compass it that at its basic usage it should work as such, like when i open the compass app on my phone and it points to north, or even if i had a real compass for that matter!!. i'd like it to point to north and the arrow point towards where the node is.. so maybe they could be the new updated options..

Hello!
In the second picture which is not backlit. Everything is as you described.
The north marker of the compass (small ring) rotates around depending on where you are going. In this case the node called buda is behind me and I am moving away from it. And my direction of travel is 81.58° east. So I can see which way is north.
You can see this if you have the "north top" switch turned off in the settings. 😉
This is how it works now, but there is no short line showing the direction of travel.

@Szetya
Copy link
Contributor Author

Szetya commented Sep 24, 2024

Looks great @Szetya ! Want to try proposing another patch :D

I have some thoughts and would like to write you a longer letter. I am still on my way home from work. But I haven't forgotten. 😇

@Szetya
Copy link
Contributor Author

Szetya commented Sep 24, 2024

Looks great @Szetya ! Want to try proposing another patch :D

I have some thoughts and would like to write you a longer letter. I am still on my way home from work. But I haven't forgotten. 😇

First. The heading is not a real heading just a heading based on GPS data, as far as I can tell, and has nothing to do with a magnetic compass. So you have to be moving to get meaningful data.
Nothing wrong with that either, I have found that despite the discrepancies due to GPS accuracy and e-ink being updated less frequently, it is quite usable.
Although the code also calls it heading let's stick with that wording. Hopefully no one is sitting backwards in their car or driving like a crab.
During the day I had the following thoughts:
-if there is no GPS locked status, can anything appear? No! And fortunately it does. There is nothing to do. ✅
-Don't show the heading sign at a station with a fixed position as it is not meaningful. Can be solved by checking "config.position.gps_mode" and/or "config.position.fixed_position". ❌
There is some inconsistency here because for FIX position "Compass north top" should be in effect. Here we are also lucky because the myHeading variable will always be zero so the north marker will be set to top. ✅
Currently, that's all that has changed in the functions. For a moving node it can be used but for a stationary station the direction line can be confusing. At the moment it is interfering now.
The drawNodeHeadnig function is unchanged as the arrow direction is now rotated according to "config.display.compass_north_top".
What happens:
I store myHeading value in the myHeadingTemp temporary variable, and then set either the north marker or the heading marker to the top according to "config.display.compass_north_top".

void Screen::drawCompassNorth(OLEDDisplay display, int16_t compassX, int16_t compassY, float myHeading)
{
// If north is supposed to be at the top of the compass we want rotation to be +0
float myHeadingTemp = myHeading;
if (config.display.compass_north_top)
myHeading = -0;
/
N sign points currently not deleted*/
Point N1(-0.04f, 0.65f), N2(0.04f, 0.65f); // N sign points (N1-N4)
Point N3(-0.04f, 0.55f), N4(0.04f, 0.55f);
Point NC1(0.00f, 0.50f); // north circle center point
Point HD1(0.00f, 0.46f), HD2(0.00f, 0.54f); // Heading line
Point *rosePoints[] = {&N1, &N2, &N3, &N4, &NC1, &HD1, &HD2};

uint16_t compassDiam = Screen::getCompassDiam(SCREEN_WIDTH, SCREEN_HEIGHT);

for (int i = 0; i < 5; i++) {
    // North on compass will be negative of heading
    rosePoints[i]->rotate(-myHeading);
    rosePoints[i]->scale(compassDiam);
    rosePoints[i]->translate(compassX, compassY);
}
if (config.display.compass_north_top) {
    HD1.rotate(myHeadingTemp);
    HD2.rotate(myHeadingTemp);
    } else {
    // Heading top
    HD1.rotate(-0);
    HD2.rotate(-0);
    }
HD1.scale(compassDiam);
HD2.scale(compassDiam);
HD1.translate(compassX, compassY);
HD2.translate(compassX, compassY);

/* changed the N sign to a small circle on the compass circle.
display->drawLine(N1.x, N1.y, N3.x, N3.y);
display->drawLine(N2.x, N2.y, N4.x, N4.y);
display->drawLine(N1.x, N1.y, N4.x, N4.y);
*/
display->drawCircle(NC1.x, NC1.y, 4); // North sign circle, 4px radius is sufficient for all displays.
display->drawLine(HD1.x, HD1.y, HD2.x, HD2.y);      // Heading sign line
display->drawString(5, 140, "Heading: " + String(myHeadingTemp * 180 / PI));   // just debug for me

}

To be honest, whoever created the original functions made it very easy to understand and logical.
I realise that at first the two functions seem confusing. Basically only the "north top" has changed in that you can see the direction of movement on the outer ring. Even the "north top" off state has only changed in that the motion diretion marker has been moved to the top.
(The start of the function is not in the editor window, it is above it.)

@HarukiToreda
Copy link
Contributor

Looks great @Szetya ! Want to try proposing another patch :D

I have some thoughts and would like to write you a longer letter. I am still on my way home from work. But I haven't forgotten. 😇

First. The heading is not a real heading just a heading based on GPS data, as far as I can tell, and has nothing to do with a magnetic compass. So you have to be moving to get meaningful data. Nothing wrong with that either, I have found that despite the discrepancies due to GPS accuracy and e-ink being updated less frequently, it is quite usable. Although the code also calls it heading let's stick with that wording. Hopefully no one is sitting backwards in their car or driving like a crab. During the day I had the following thoughts: -if there is no GPS locked status, can anything appear? No! And fortunately it does. There is nothing to do. ✅ -Don't show the heading sign at a station with a fixed position as it is not meaningful. Can be solved by checking "config.position.gps_mode" and/or "config.position.fixed_position". ❌ There is some inconsistency here because for FIX position "Compass north top" should be in effect. Here we are also lucky because the myHeading variable will always be zero so the north marker will be set to top. ✅ Currently, that's all that has changed in the functions. For a moving node it can be used but for a stationary station the direction line can be confusing. At the moment it is interfering now. The drawNodeHeadnig function is unchanged as the arrow direction is now rotated according to "config.display.compass_north_top". What happens: I store myHeading value in the myHeadingTemp temporary variable, and then set either the north marker or the heading marker to the top according to "config.display.compass_north_top".

void Screen::drawCompassNorth(OLEDDisplay display, int16_t compassX, int16_t compassY, float myHeading) { // If north is supposed to be at the top of the compass we want rotation to be +0 float myHeadingTemp = myHeading; if (config.display.compass_north_top) myHeading = -0; / N sign points currently not deleted*/ Point N1(-0.04f, 0.65f), N2(0.04f, 0.65f); // N sign points (N1-N4) Point N3(-0.04f, 0.55f), N4(0.04f, 0.55f); Point NC1(0.00f, 0.50f); // north circle center point Point HD1(0.00f, 0.46f), HD2(0.00f, 0.54f); // Heading line Point *rosePoints[] = {&N1, &N2, &N3, &N4, &NC1, &HD1, &HD2};

uint16_t compassDiam = Screen::getCompassDiam(SCREEN_WIDTH, SCREEN_HEIGHT);

for (int i = 0; i < 5; i++) {
    // North on compass will be negative of heading
    rosePoints[i]->rotate(-myHeading);
    rosePoints[i]->scale(compassDiam);
    rosePoints[i]->translate(compassX, compassY);
}
if (config.display.compass_north_top) {
    HD1.rotate(myHeadingTemp);
    HD2.rotate(myHeadingTemp);
    } else {
    // Heading top
    HD1.rotate(-0);
    HD2.rotate(-0);
    }
HD1.scale(compassDiam);
HD2.scale(compassDiam);
HD1.translate(compassX, compassY);
HD2.translate(compassX, compassY);

/* changed the N sign to a small circle on the compass circle.
display->drawLine(N1.x, N1.y, N3.x, N3.y);
display->drawLine(N2.x, N2.y, N4.x, N4.y);
display->drawLine(N1.x, N1.y, N4.x, N4.y);
*/
display->drawCircle(NC1.x, NC1.y, 4); // North sign circle, 4px radius is sufficient for all displays.
display->drawLine(HD1.x, HD1.y, HD2.x, HD2.y);      // Heading sign line
display->drawString(5, 140, "Heading: " + String(myHeadingTemp * 180 / PI));   // just debug for me

}

To be honest, whoever created the original functions made it very easy to understand and logical. I realise that at first the two functions seem confusing. Basically only the "north top" has changed in that you can see the direction of movement on the outer ring. Even the "north top" off state has only changed in that the motion diretion marker has been moved to the top. (The start of the function is not in the editor window, it is above it.)

I did notice that the waypoint screen has the circle missing around the compass. I use inverted header, not sure if that's what causes it.

20240924_155810

@Szetya
Copy link
Contributor Author

Szetya commented Sep 25, 2024

I did notice that the waypoint screen has the circle missing around the compass. I use inverted header, not sure if that's what causes it.

20240924_155810

Hi!
What a good observation! For the reverse header, it draws a dark filled rectangle at the top on display and then writes on it with the background color.
display->setColor(BLACK); stays on after writing the header and draws the outer ring of the compass in black. 😉
This is more than likely how it was before. I would like to look at it at home this afternoon.

@Szetya
Copy link
Contributor Author

Szetya commented Sep 25, 2024

I did notice that the waypoint screen has the circle missing around the compass. I use inverted header, not sure if that's what causes it.
20240924_155810

Hi! What a good observation! For the reverse header, it draws a dark filled rectangle at the top on display and then writes on it with the background color. display->setColor(BLACK); stays on after writing the header and draws the outer ring of the compass in black. 😉 This is more than likely how it was before. I would like to look at it at home this afternoon.

I cannot confirm this error at the moment. I have tried all options for the display setting but the circle is always visible.
Before line 1506 in /src/graphics/Screen.cpp (display->drawCircle...) you could fix the white (actually green) color by adding a line. Here the circle is drawn completely independent of the cursor and pointers.
display->setColor(WHITE);
IMG_20240925_153207_edit_4662786170300491

@HarukiToreda
Copy link
Contributor

I did notice that the waypoint screen has the circle missing around the compass. I use inverted header, not sure if that's what causes it.
20240924_155810

Hi! What a good observation! For the reverse header, it draws a dark filled rectangle at the top on display and then writes on it with the background color. display->setColor(BLACK); stays on after writing the header and draws the outer ring of the compass in black. 😉 This is more than likely how it was before. I would like to look at it at home this afternoon.

I cannot confirm this error at the moment. I have tried all options for the display setting but the circle is always visible. Before line 1506 in /src/graphics/Screen.cpp (display->drawCircle...) you could fix the white (actually green) color by adding a line. Here the circle is drawn completely independent of the cursor and pointers. display->setColor(OLEDDISPLAY_COLOR:WHITE) ; IMG_20240925_153207_edit_4662786170300491

Happens on the waypoint screen, you have to set a waypoint on the map to show that screen

@Szetya
Copy link
Contributor Author

Szetya commented Sep 25, 2024

I did notice that the waypoint screen has the circle missing around the compass. I use inverted header, not sure if that's what causes it.
20240924_155810

Hi! What a good observation! For the reverse header, it draws a dark filled rectangle at the top on display and then writes on it with the background color. display->setColor(BLACK); stays on after writing the header and draws the outer ring of the compass in black. 😉 This is more than likely how it was before. I would like to look at it at home this afternoon.

I cannot confirm this error at the moment. I have tried all options for the display setting but the circle is always visible. Before line 1506 in /src/graphics/Screen.cpp (display->drawCircle...) you could fix the white (actually green) color by adding a line. Here the circle is drawn completely independent of the cursor and pointers. display->setColor(OLEDDISPLAY_COLOR:WHITE) ; IMG_20240925_153207_edit_4662786170300491

Happens on the waypoint screen, you have to set a waypoint on the map to show that screen

I can't get this screen to pop up. :) Is it possible with an iPhone app?

@HarukiToreda
Copy link
Contributor

I did notice that the waypoint screen has the circle missing around the compass. I use inverted header, not sure if that's what causes it.
20240924_155810

Hi! What a good observation! For the reverse header, it draws a dark filled rectangle at the top on display and then writes on it with the background color. display->setColor(BLACK); stays on after writing the header and draws the outer ring of the compass in black. 😉 This is more than likely how it was before. I would like to look at it at home this afternoon.

I cannot confirm this error at the moment. I have tried all options for the display setting but the circle is always visible. Before line 1506 in /src/graphics/Screen.cpp (display->drawCircle...) you could fix the white (actually green) color by adding a line. Here the circle is drawn completely independent of the cursor and pointers. display->setColor(OLEDDISPLAY_COLOR:WHITE) ; IMG_20240925_153207_edit_4662786170300491

Happens on the waypoint screen, you have to set a waypoint on the map to show that screen

I can't get this screen to pop up. :) Is it possible with an iPhone app?

good question, if you go to the map, can you hold on a randome spot on the map and see if allows the waypoint to be dropped? I've never tried on ios

@Szetya
Copy link
Contributor Author

Szetya commented Sep 25, 2024

good question, if you go to the map, can you hold on a randome spot on the map and see if allows the waypoint to be dropped? I've never tried on ios

I have the Android 2.4.13 application. If I hold my finger on a node it jumps to that node in the node list. Unbelievable but I can't find this option. :)

@HarukiToreda
Copy link
Contributor

HarukiToreda commented Sep 25, 2024

good question, if you go to the map, can you hold on a randome spot on the map and see if allows the waypoint to be dropped? I've never tried on ios

I have the Android 2.4.13 application. If I hold my finger on a node it jumps to that node in the node list. Unbelievable but I can't find this option. :)
here's a video

Screen_Recording_20240925_115713_Meshtastic.mp4

@Szetya
Copy link
Contributor Author

Szetya commented Sep 25, 2024

good question, if you go to the map, can you hold on a randome spot on the map and see if allows the waypoint to be dropped? I've never tried on ios

I have the Android 2.4.13 application. If I hold my finger on a node it jumps to that node in the node list. Unbelievable but I can't find this option. :)
here's a video

Screen_Recording_20240925_115713_Meshtastic.mp4

This circle is drawn in src/modules/WayPointModule.cpp and that's where the error is.
Before drawing the circle, if you inverted the display, it would set the color to black. The if and drawCircle need to be changed. 😉
IMG_20240925_190702_edit_4668600485912104

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.

6 participants