Renderer/HorizonRenderer: more elaborate artificial horizon

[qubolino]

more elaborate artificial horizon with standard colorblind-friendly colors, shape and marks

Brief summary of the changes

Related issues and discussions

[MaxKellermann]

You requested a review, but your code was already reviewed in #1261 and apparently you didn't obey the review. I wish you hadn't closed #1261 because now we're wasting time on doing the same again.

[qubolino]

i did spend quite some time complying to what lordfolken advised.

if that still needs work to meet your standards, i suggest you do it yourself, or ditch my work that i happily tried to share with the community

[MaxKellermann]

i did spend quite some time complying to what lordfolken advised.

Just look at the very first review comment: #1261 (comment)

Then look at this piece in this PR you just submitted:

   // sky_pen.Create(Layout::Scale(1), LightColor(sky_color));

The is the very line @lordfolken criticized, and it's still there. You didn't reply to his comment and you did not fix this.

I stopped reading your PR after seeing this, but I should have stopped reading after seeing that your commit message doesn't even explain what your proposed change is about.

[MaxKellermann]

Oh no, now you suggest merging a merge commit. Everything is wrong with this PR.

I suggest you should continue working on #1261 and not this one, because over there, you see unresolved review comments.

Your last post sounded like you don't want to do that. That's okay for me, you can do whatever you want, and you don't need to listen to us. But it might cause us not listening to you either.

[lordfolken]

Thank you for sticking with it. A few minor points in my current review.
Next i'll try to compile and run your code.

[lordfolken]

Git history:
we like rebased code on current master. Merges aren't optimal.
do a
git pull origin master --rebase on your branch, commit your code and do a force push here.

[lordfolken]

Max isn't very tolerant of when things aren't done in the way he expects.
He is however fair when the code is well written (and i thnk the code is clear and good) and will merge it.
I'll try to help get things into shape with you.
So just stick with it and don't get discouraged.

[lordfolken]

@MaxKellermann i'll reopen, when i think it looks presentable.

[lordfolken]

@iltis42 and @XCNav can you have a look at this? i think your customers would benefit mostly.

[MaxKellermann]

Max isn't very tolerant of when things aren't done in the way he expects.

First of all, I expect people to handle the existing reviews before asking for another review. @qubolino ignored the previous review, while pretending he "complied" (which I proved wrong). Then he explained he didn't want to meet our standards and asked me to do it for him. I certainly won't do that.

And this isn't event arguing whether commented line would be acceptable. At this point, this isn't about the code at all. It's about @qubolino's behavior towards code reviews.

[qubolino]

dear max. i suggest you try be more tolerant on others skills.

i respect your work, and you probably are a very skilled programmer.

i opened a new pr b/c i've been asked to put everything in one commit. i don't know how to do this working with an existing pr. i actually don't know how to work with git as you already know. in fact i don't know how to do many things.

one thing i know though is how to propose the perhaps only mediocre result of my work to the community -- despite the fact i benefited from a surely high standard result of its work. this pr was initially more a pull proposition than a request. i initially didn't envision to do anything more on it. then i thought it was maybe not too hard to comply and could make an effort and i did.

[qubolino]

Max isn't very tolerant of when things aren't done in the way he expects.

He is however fair when the code is well written (and i thnk the code is clear and good) and will merge it.

I'll try to help get things into shape with you.

So just stick with it and don't get discouraged.

he isn't very tolerant of probably many more things than this.
he imvho has a gift though. he is very good at discouraging people to contribute to something he doesn't own. the community does. fine by me. three times i interacted with him, three times i seemed to be completely wrong and felt my work was treated disrespectfully.

[MaxKellermann]

@qubolino you're trying to reframe your failure to read review comments carefully, making it sound like it's about me. Please don't make this personal. This doesn't help, you're only wasting more of everybody's time with a pointless off-topic discussion that leads nowhere.

(You have annoyed me enough that I'm going to unsubscribe from this PR and I'm not going to spend more time on reviewing it. Bye.)

[qubolino]

i find your behaviour very childish and non-constructive. but that's only my opinion.

speaking about being constructive, @lordfolken thanks a lot for (twice already) trying to encourage me to carry on. i'll be happy to work with you.

[qubolino]

@lordfolken thanks for your comments, i addressed them, i think.

looking fwd to your constructive critics

[DanD222]

@qubolino
If you look here https://github.com/XCSoar/XCSoar/pull/1266/commits, you'll see two new commits called "addressing" - you want to avoid that (as well as the "Merge branch 'XCSoar:master' into master" commit). If the PR was merged in this state, then XCSoar's commit history would include a whole bunch of "addressing" commits, as well as the wrong commits they fix.

What you might want to do instead is squash the "addressing" commit with the commit where the code that you're addressing is - it then becomes one commit with the right/addressed code.

After that, you Force Push from your local machine to your online Git repo - this rewrites the commits in the Pull Request, gets rid of the extraneous commits, and provides a nice commit set for review (providing the commit history is correct on your machine in the first place).

Regarding the "Merge branch 'XCSoar:master' into master" commit, you want to Rebase your branch on origin/master instead - this brings your branch up to date, but doesn't include the unnecessary merge commit.

(Not sure what your workflow is and sorry if I'm stating the obvious, but you can't do any of this through the Github web UI - this needs to be done on your local machine through an IDE/Git GUI/Git commands)

[qubolino]

i squashed everything in one commit

[lordfolken]

Finally got around to testing it. So the main window looks good to me and seems to behave properly. The Attitude Infobox, has some errors. Its not filling the whole square and the lines are not adapting to the smaller resolution.
Your commit is missing a new line at the end.

[lordfolken]

Finally got around to testing it. So the main window looks good to me and seems to behave properly. The Attitude Infobox, has some errors. Its not filling the whole square and the lines are not adapting to the smaller resolution.
Your commit is missing a new line at the end.

[iltis42]

Also tested a bit meanwhile. IMHO the yellow arrow should as well move with the bank as long the yaw is not changing, see the last picture i have attached. Same in the small infobox. The small infobox is overloaded, an idea was to remove the yaw indication there, or remove the details and use smaller arrows, maybe only arrows. The gauge is a bit to much in here.

[qubolino]

the yellow arrow should as well move with the bank as long the yaw is not changing
in old mechanical (but also traditionally) the yellow is not moving relative to the plane (nor is anything due to the yaw)
The small infobox is overloaded
agreed and addressed. i kept only ground sky and plane in yellow

[qubolino]

the yellow arrow should as well move with the bank as long the yaw is not changing

in old mechanical (but also traditionally) the yellow is not moving relative to the plane (nor is anything due to the yaw)

The small infobox is overloaded

agreed and addressed. i kept only ground sky and plane in yellow

[XCNav]

Here you can see how yaw is handled at modern EFIS Aircraft Avionics. The yaw indicator is the little white box below the white bank angle arrow. They both move basically in one unit. As the Aircraft slips the white box is diverting from its position to the left or to the right. Maximum deflection is one bix width. Is the nose yawing to the left, the bar is diverting to the right and vice versa.

[qubolino]

Here you can see how yaw is handled at modern EFIS Aircraft Avionics

what you describe isn't yaw https://en.wikipedia.org/wiki/Yaw_(rotation).
it's a slip indicator.
and there are different conventions for different efis.

my implementation of the horizon only takes into account roll and pitch.
and i chose the convention of yellow isn't moving relative to aircraft.
https://en.wikipedia.org/wiki/Attitude_indicator

[lordfolken]

@qubolino if you are still around, can you have a look?