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

SID tag colouring, new tag item additions, code optimisation #52

Merged
merged 15 commits into from
Dec 11, 2019

Conversation

Keanu73
Copy link
Contributor

@Keanu73 Keanu73 commented Dec 7, 2019

This PR fixes #22; here's a current changelog:

- Reworks config - background colours for tags are now set by a default object inside a background_colours object, which also replaces background_on_runway with a runway object and also adds a nosid object. See b93be07 for more. (sorry keanu, but no config reworks today)

  • Adds SID tag colouring functionality (see here for an example)

  • Removes communication type menu from callsign tag item & changes right-click item on callsign tag item to handoff menu based on feedback; also lets you properly select the tag by left-clicking on tag items

  • Adds 4 new tag items - arvrwy (arrival runway), srvrwy (speed that changes to runway < 25 knots), origin (ICAO origin aerodrome), and dest (ICAO destination aerodrome). Also renames asid to sid and ssid to shid. (ah well)

  • Optimises code in some parts (see 40b62bc and d853667)

  • Changes default airport (sorry but I'm not familiar with france's SIDs, gatwick was a better testing airport for SID tag colouring)

And of course, these changes have been fully tested by me and another person with great success.

@Keanu73
Copy link
Contributor Author

Keanu73 commented Dec 7, 2019

After this initial merge, I plan to work on a complete refactoring of the code and reduce the amount of if statements in SMRRadar.cpp.. always good to do a code cleanup :)

@Keanu73
Copy link
Contributor Author

Keanu73 commented Dec 8, 2019

How we doing @pierr3?

@pierr3
Copy link
Owner

pierr3 commented Dec 8, 2019

Hey, thank you so much for your contribution, really appreciate your work.

I cannot merge as is in order to maintain quality, realism and compatibility. A few things that I would need to see changed before we can merge:

  • A lot of your changes are not backward compatible with existing configs, and will cause problems for most users when trying to upgrade. Requiring a config rewrite at this stage is not something I want yet.
  • "renames asid to sid and ssid to shid" is for example a config breaking change that does not enhance user experience
  • Same for background_colors, good change that makes sense but will break config, please
  • "Removes communication type menu from callsign tag item" has this been moved to right click?
  • Your current default config is too big to ship as is, please simplify in the same way as the old demo config

Any ETA on when you'll have all the changes you want to merge?

@Keanu73
Copy link
Contributor Author

Keanu73 commented Dec 9, 2019

First of all, thanks for making me realise my habits. Sometimes I think "ooh, this would look better" without thinking how it would break other users' configs. I would consider making a config migration script, but I first need to get around to refactoring the plugin.

As for the communications type menu thing, I didn't move it to right click, as I replaced that with the handoff menu. When asking for feedback from other users, the handoff menu being on left click often put them in a bad spot, as they would usually think that clicking on the callsign would select the aircraft, but instead they accidentally assume the tag. When I also considered swapping them around, I thought that it wouldn't really provide a tangible advantage either, so I just decided to remove it entirely. Though, I could bind the communication type menu to the middle mouse button if you want.
(I also didn't think about reading the NOVA's manual :P)

I understand your concerns about the config, it is definitely too big as a user suggested having a "Default" and a "Pro" profile by default, but I hadn't thought that well this is just a config to kickstart the user, it's not meant to provide everything for them. (Do you reckon I should shorten the sids list in the config anyway?)

Also, do you think aircraft that have filed a flightplan for circuits should be treated as departures or arrivals?

I will definitely get these fixed either by today or tomorrow or the day after. No later than that.

After this PR, I'll be working on refactoring the whole plugin by doing things such as seperating the SMRRadar.cpp file to be not as big as it is, and I'm also considering doing a config rework if that's okay with you. I'll also probably be adding things like a GUI config editor in the plugin so that those who are not that tech-savvy to use JSON can just pick the colours they want from within the plugin, and without having to type .smr reload or restart EuroScope.

Also, it really really gladdens me to see that you still maintain this project, so thank you so much 😄

…communication type menu to right click and changes handoff menu to middle mouse button
@Keanu73
Copy link
Contributor Author

Keanu73 commented Dec 9, 2019

Current changelog:

  • Made config smaller, reverted config rework

  • Reverted tag item renaming (shid is now ssid and sid is now asid)

  • Added communication type menu back to right-click and changed handoff menu to middle mouse button only

@Keanu73
Copy link
Contributor Author

Keanu73 commented Dec 9, 2019

Making sure not to get rid of DrFreas' changes in my PR :)

Copy link
Owner

@pierr3 pierr3 left a comment

Choose a reason for hiding this comment

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

Current changelog:

  • Made config smaller, reverted config rework
  • Reverted tag item renaming (shid is now ssid and sid is now asid)
  • Added communication type menu back to right-click and changed handoff menu to middle mouse button only

Thanks for this! I would say change handoff to right click and voice to middle click if you can (I'll do it otherwise) This is because handoff is more likely to be used than change voice.

vSMR/SMRRadar.cpp Outdated Show resolved Hide resolved
@Keanu73
Copy link
Contributor Author

Keanu73 commented Dec 11, 2019

I've done both of your requests, and it should all be good for merge now. :D

@pierr3
Copy link
Owner

pierr3 commented Dec 11, 2019

Thanks for this, merging, please make sure the docs are up to date and I'll do the same with a bit more thorough review soon

@pierr3 pierr3 merged commit 3434538 into pierr3:master Dec 11, 2019
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.

Outbound background colors.
2 participants