STARS v2: Part 1 #30

checkandmate1 · 2024-03-06T00:42:46Z

Lots of new features. Probably should be a beta release because there may be a few bugs that when running a local server and two instances of vice at the same time on a MacBook were not found:

Specifying positions
- Same-sector positions can be specified with one letter (eg. A to handoff to 4A working 4P
- Interfacility positions must be specified with the delta (∆) symbol, the facility identifier, and then the sector ID
- If specified in the facility configuration file, C can be used as a handoff to the appropriate center controller
- If specified in the facility configuration file, interfacility handoffs can be specified with the delta symbol and the facility ID
Handoffs
- More realism with the syntax required for certain handoffs
Misc. STARS Commands
- ** will Force QL the target to the user, if they have control of the track and it is allowed by the STARS facility (specified in the facility configuration file)
- ** + SECTORID will force QL a track to that intrafacility position
- MULTI FUNC, O will display the last 20 TCPs that have accepted a point out
- QL a TCP without needing to use MULI FUNC, Q (yay!)
- Scratchpads may now be set with SLEW instead of MULTI FUNC, Y The Length of scratchpads defaults at 3, but can be extended to four if allowed by the STARS facility. (Specified in the facility configuration file)
Facility configuration file changes
- The Controller struct now needs a facility_identifier JSON field
- NewSimConfigration now needs the stars_adaptation JSON field containing: a bool: force_ql_self, a [2]bool: scratchpad_rules bool[0] is for the primary scratchpad, bool[1] is for the secondary scratchpad, and the airspace_awareness struct. Inside of this struct there is a []string: fix which is a slice of fixes that an aircraft could be filed over. A string altitude_range which is specified as 100450 for 10,000 to 45,000. A stringreceiving_controller which is the controller that will receive this. Finally the book: to_center which is pretty self-explanatory
TODO List
- ~~Redirecting Handoffs~~
- MULTI FUNC ZZ
- Unassociated tracks
- ~~LDB vs PDB~~
- Needing to specify the BCN or ACID to initiate control
- Editing who gets what during multiplayer sessions. (So that CAMRN doesn't have to work Yardley if HAARP is online, the users can decide)
- DCB size is not one set size but depends on the screen size (My DCB takes up the entire top of my screen, while Jud's for example is 1/3 - 1/2 his screen size
- ~~Global Leader Lines~~
Fixes
- Fixed aircraft going direct after being handed off to a real controller

Let me know if anything should be changed (probably a lot that is going to be needed too)!

catcarbon · 2024-03-21T15:35:33Z

sim.go

@@ -2456,7 +2456,9 @@ type HeadingArgs struct {
 func (s *Sim) AssignHeading(hdg *HeadingArgs) error {
 	s.mu.Lock(s.lg)
 	defer s.mu.Unlock(s.lg)
-
+	if hdg.Heading%5 != 0 { // Just so that scratchpads like R11 can go in
+		return errors.New("Heading too specific")


Not meant to block this PR but I think tweaks like this are telltale that we need to make simulator commands explicitly distinct from STARS commands.

This would be a good idea. Two questions I have:

What would be a good toggle/ prefix to do this with?

What if that aircraft command doesn't exist? What would be returned?

(This was done in 22225c2, so the check here can go.)

mmp · 2024-03-24T14:11:00Z

stars.go

+	}
+	if lc > 4 || (lc == 4 && !ctx.world.STARSFacilityAdaptation.ScratchpadRules[0] && !isSecondary) ||
+		(lc == 4 && !ctx.world.STARSFacilityAdaptation.ScratchpadRules[1] && isSecondary) {
+		sp.previewAreaOutput = GetSTARSError(ErrSTARSIllegalScratchpad).Error()


Two general comment about error flows (applies here and elsewhere):

Whenever possible, prefer returning an error code than doing straight to setting sp.previewAreaOutput. In that way, the caller of the function can see that an error occurred and decide what to do about it, possibly handling the error or possibly showing it to the user. The main exception to this is the error handling callbacks for RPCs, which pretty much have no other alternative than to show it to the user.

Related, GetSTARSError is only needed to remap non-STARS error types (e.g. strconv.Atoi returning an error) to a STARS error code for display in the preview area. So there's no need to call that when you know for sure you have a STARS error code.

mmp · 2024-03-24T14:19:36Z

stars.go

+			}
+		}
+		if callsign == "" {
+			return "", errors.New("Cannot calculate airspace")


It's probably better to return ErrSTARSIllegalPosition here (and similarly elsewhere in the function). It looks like the calling code is either checking if the error is nil or not, or is returning ErrSTARSIllegalPosition itself.

Actually, I think it would be better to return just a *Controller from this function, with nil for none found. Some of the calling code turns around and calls world.GetController() with the returned callsign, which is an unnecessary round-trip, and otherwise just shows an illegal position error if none is found. So a nil pointer is sufficient to convey the only failure case here.

mmp · 2024-03-24T14:25:50Z

stars.go

-			controller := singleScope(ctx, string(controller[2]))
-			if controller != nil {
-				return true, controller.Callsign
+	if controller[0] == 'C' || (haveTrianglePrefix && lc == 3) {


Regarding the lc==3 test: is this implicitly baking in the knowledge that the len of STARSTriangleCharacter is 2? (Similar question about the lc == 5 && haveTrianglePrefix test below.)

I think it would be better to always call strings.TrimPrefix(controller, STARSTriangleCharacter) right after HasPrefix(controller, STARSTriangleCharacter) at the start of the function (and then move lc := len(controller) after that . Then following code can purely use haveTrianglePrefix to distinguish that case and can then operate on string lengths that are based on the remaining non-triangle text.

The 3 and 5 did have the length of the triangle character in mind. What I did before was if haveTrianglePrefix was true, lc -= 1. I could do that here, or just trim the prefix and get the length of that. One issue I see with trimming the prefix is that haveTrianglePrefix is used multiple times during calculateController, so trimming it at the beginning may lead to some problems with checking the other handoffs that could have been done.

mmp · 2024-03-24T14:39:23Z

stars.go

@@ -3133,6 +3162,9 @@ func (sp *STARSPane) handoffTrack(ctx *PaneContext, callsign string, controller
 // and route.
 func calculateAirspace(ctx *PaneContext, callsign string) (string, bool) {


I think it would work better to return *AirspaceAwareness here (with nil indicating "couldn't find anything"). Returning a magic string "no target" is a little error prone / obscure (e.g. it could be misspelled here or there without that being realized), and looking at the calling code, it ends up returning an error in any case when AirspaceAwareness isn't found. (More comments below where it's used.)

If details of why the function failed were needed, it would be better to use an error and to declare a global variable for the error code or codes so that calling code could use == tests to see what the actual error was.

There is one thing about this. All errors are the same because we're returning nil. For example, if the ACID/BCN was entered incorrectly, the error code should be NO FLIGHT; but, if the receiving position does not exist, the error should be ILL POS. I think changing it to a string and an error would be the way to go.

mmp · 2024-03-24T14:45:16Z

stars.go

+			return "", errors.New("Cannot calculate airspace")
+		}
+		if controller == "C" {
+			control, toCenter := calculateAirspace(ctx, callsign)


Looking at the flow of how these return values are used currently, "no target" returns an error. For the other failure case in calculateAirspace (not finding a match), "", false is returned; in turn, world.GetController() will fail and an error is returned as well.

It should never happen that a match is found in calculateAirspace but passing rules.ReceivingController to world.GetController will fail; ~~we validate that in scenario.go when the scenario is loaded~~. We have commented-out code in scenario.g (line 696) that validates this; it currently fails since most scenarios are still using sector ids and not callsigns there.

Anyway, once the scenarios are updates and we validate all of the ReceivingController callsigns at scenario load time, then it's reasonable to assume that world.GetController() will always succeed for a valid AirspaceAwareness.

mmp · 2024-03-24T14:51:58Z

stars.go

@@ -3175,76 +3207,86 @@ func singleScope(ctx *PaneContext, facilityIdentifier string) *Controller {

 // Give a bool if the handoff is good and the correct syntax.
 // Also decode the controller into its regular sector (N4P -> 4P)
-func (sp *STARSPane) calculateController(ctx *PaneContext, controller, callsign string) (bool, string) {
+func (sp *STARSPane) calculateController(ctx *PaneContext, controller, callsign string) (string, error) {


This function is a little tricky to understand (in part because it's inherently complex how this is resolved), but I think renaming the controller parameter would help: it is actually.. a TCP ID? Or is there a better word to describe what the user's input corresponds to? controller suggests to me at least that the variable is either a controller callsign or a Controller object. Anyway, I think a more distinct name that makes it more clear that this is the thing the user entered that is being mapped to a control position would make the function easier to understand.

mmp

Overall looks good; a few small requests..

stars.go

aircraft.go

mmp · 2024-05-05T00:57:24Z

sim.go

-			s.lg.Infof("%s/%s/%s: launch departure", airport, runway, category)
-			s.launchAircraftNoLock(*ac)
-			s.NextDepartureSpawn[airport] = now.Add(randomWait(rateSum, false))
+		for { // There will always be a gate, because is all gates are checked this func wont get called


From before I thought that this wasn't true? Wasn't there a case something like if the scenario has 0 departures to some gate then if every other gate but that one is stopped but that one isn't, then it still won't be able to launch any aircraft?

If so, would it work to just continue to call spawnAircraft(), let it fail if the gate it chose is stopped, and then assume it will eventually find a gate that's open (if there is one) in the next few times...?

Do you mean if there are no departures made/ coded in for that one gate?

No, I mean for example, say I launch the "KEWR 4L Departure" scenario and set an ADR of 0 for KMMU and KTEB when I create the sim. Then if I halt departures from KEWR 4L I think this will go into an infinite loop because yes, there are some gates that aren't stopped, but on the other hand a departure from them will never be launched.

I tried to reproduce this and looks like it was fixed in 0eb5aec.

mmp · 2024-05-05T00:57:55Z

sim.go

@@ -2456,7 +2456,9 @@ type HeadingArgs struct {
 func (s *Sim) AssignHeading(hdg *HeadingArgs) error {
 	s.mu.Lock(s.lg)
 	defer s.mu.Unlock(s.lg)
-
+	if hdg.Heading%5 != 0 { // Just so that scratchpads like R11 can go in
+		return errors.New("Heading too specific")


(This was done in 22225c2, so the check here can go.)

sim.go

checkandmate1 and others added 30 commits February 19, 2024 13:47

Added live wx

5cc3c60

Merge branch 'mmp:master' into live-wind

5d71f81

Fixed crash bug

d195b63

Improved Preformance

22438ad

Logging Fix

512a50a

Changed location of airportWind

d3a69f8

CWT

3521f08

ghost targets i think

1d6994b

Merge branch 'mmp:master' into cwt

d47e7a2

Alitude filter bugfix

a65a19c

Fixed altitude filters

7f0dc12

Fixed altitude filters & finished CWT

e066099

VRB fix pt1

48257f4

Incoming Handoffs bypass altitude filters

0107dc9

Removed print

5bf3f8a

Fixed pointouts

d95753c

Added VRB compatibility

201aa86

Merge branch 'live-wind' into cwt

d8306d8

Fixed wind direction bug

ebd3618

Altitude Bypass for ATPA and CAs

1f9752c

Minor Improvements

0cf6bc1

Added PHL to CWT

8ce2680

Updated RECAT to CWT

4593e3b

Removed recat_facility

b8376d3

Removed recat_facility

ea4213f

uiDisable the refresh weahter button when live wx isn't selected

a64f2de

Added airspace awareness rules

ade700f

Polished center handoffs

4b7d640

Added center and same facility handoff logic

3492bb5

Finished intrafacility handoffs and remvoed rejecting handoffs

7cc04cd

checkandmate1 added 3 commits March 21, 2024 08:43

gofmt

bd56a9b

Global SPC

d96ac1c

gofmt

ec92896

catcarbon reviewed Mar 21, 2024

View reviewed changes

checkandmate1 mentioned this pull request Mar 21, 2024

STARS: should issue an error if we attempt to hand off to ourself #150

Closed

checkandmate1 and others added 9 commits March 21, 2024 19:46

Fixed handoffs

d12daa3

Error checking for invalid control positions

860c9e2

Polished the formatting

73ac8bd

gofmt

579e75b

Rolled back scopeChars (for now)

3fa5fb8

go fmt

c5def4c

Added vice executable

b3a5276

Change base trackID

e75ddb0

Merge branch 'master' into mtrokel-pr30

c28915c

mmp reviewed Mar 24, 2024

View reviewed changes

checkandmate1 mentioned this pull request Mar 25, 2024

Fix handling of various CRDA commands #159

Merged

mmp and others added 4 commits April 5, 2024 06:32

Merge branch 'master' into mtrokel-pr30

7aead61

Remove unused warnings code

751d424

Merge branch 'master' into mtrokel-pr30

4b4cc4a

Merge branch 'master' into airspace-awareness

538381f

mmp reviewed May 5, 2024

View reviewed changes

checkandmate1 added 4 commits May 10, 2024 22:52

ToggleFP into World instead of Aircraft

49b9235

Remove heading check

5301de4

Remove comment

2a026c7

Fixed infinite-loop

0eb5aec

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

STARS v2: Part 1 #30

STARS v2: Part 1 #30

checkandmate1 commented Mar 6, 2024 •

edited

Loading

catcarbon Mar 21, 2024

mmp Mar 23, 2024

checkandmate1 Mar 25, 2024 •

edited

Loading

mmp May 5, 2024

mmp Mar 24, 2024

mmp Mar 24, 2024

mmp Mar 24, 2024

mmp Mar 24, 2024

checkandmate1 Mar 25, 2024

mmp Mar 24, 2024

checkandmate1 Mar 25, 2024

mmp Mar 24, 2024

mmp Mar 24, 2024

mmp left a comment

mmp May 5, 2024

checkandmate1 May 11, 2024

mmp May 13, 2024

checkandmate1 May 23, 2024

mmp May 5, 2024

		@@ -3133,6 +3162,9 @@ func (sp STARSPane) handoffTrack(ctx PaneContext, callsign string, controller
		// and route.
		func calculateAirspace(ctx *PaneContext, callsign string) (string, bool) {

STARS v2: Part 1 #30

Are you sure you want to change the base?

STARS v2: Part 1 #30

Conversation

checkandmate1 commented Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

checkandmate1 Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

checkandmate1 commented Mar 6, 2024 •

edited

Loading

checkandmate1 Mar 25, 2024 •

edited

Loading