pc2ccs / pc2v9

Version 9 of the PC^2 Programming Contest Control System
Eclipse Public License 2.0
46 stars 23 forks source link

Update NSA to use Team Scoreboard Display Format to format team names #708

Closed lane55 closed 1 year ago

lane55 commented 1 year ago

Is your feature request related to a problem?

The NewScoringAlgorithm (NSA) does not use the Team Scoreboard Display Format for team names.

Whereas the DSA does use the Team Scoreboard Display Format and that is refected on the scoreboaed html and Standings tab

Note Issues #266 and #288 provided support for defining a Team Scoreboard Display Format string in both contest.yaml and via the Admin GUI

Feature Description:

Add support for the Team Scoreboard Display Format for team names in the NSA.

Have you considered other ways to accomplish the same thing?

No other way.

Do you have any specific suggestions for how your feature would be **implemented in PC^2?**

Like in the DSA use the following method to format the team name

ScoreboardVariableReplacer.substituteDisplayNameVariables(teamVarDisplayString, account, group));

Additional context:

lane55 commented 1 year ago

Coded, pushed to lane55 branch i_708_NSA_use_Team_Scoreboard_Display_Format

lane55 commented 1 year ago

Next task is to figure out which other parts of the system may be affected, EF, the API/WTI ? and also change NSAStandingsPane to not use MCLB

lane55 commented 1 year ago

This may have an effect of the WTI now showing team name/format per Team Scoreboard Display Format

lane55 commented 1 year ago

This change also may cause the EF to show team names based on Team Scoreboard Display Format.

lane55 commented 1 year ago

Task 2 - see if this change also (properly) affects the team API end point

clevengr commented 1 year ago

I don't have any problem per se with the suggested change, which (although as noted needs be checked for effects on the WTI and the EF) does seem to be somewhat innocuous and an improvement. However, I'm quite concerned about the fact that we in essence appear to have two different classes being used to compute scores (DefaultScoringAlgorithm and NewScoringAlgorithm). Is there a reason we need the NewScoringAlgorithm? If so, is there a reason we don't remove DefaultScoringAlgorithm and replace it (everywhere) with NewScoringAlgorithm?

lane55 commented 1 year ago

The change in NSA is not related to a change in the event feed. The team name currently is fetched from the display name (only).

lane55 commented 1 year ago

I have been mildly concerned about having two scoring algorithms for over 10 years, and I have mentioned it a number of times.

lane55 commented 1 year ago

An analysis and testing would be required to replace DSA with NSA to see if they are identical or not. DSA is both ugly and authoritative. Another concern that I have voiced over the years.

lane55 commented 1 year ago

In my testing WTI uses NSA and the change is reflected in the WTI Scoreboard "immediately" (upon refresh)

lane55 commented 1 year ago

closing issue - was merged

clevengr commented 1 year ago

In my testing WTI uses NSA and the change is reflected in the WTI Scoreboard "immediately" (upon refresh)

I do not understand this comment (I think either the comment or your testing is wrong). The WTI Scoreboard is handled in the "WTI-API" project. There are zero references in that entire project to class NewScoringAlgorithm. On the other hand, there are over a dozen references to DefaultScoringAlgorithm, including this one at lines 158-159 (on the current Develop branch) which constructs the DSA used by the WTI:

        logger.fine("Constructing DSA for Contest Controller");
        dsa = new DefaultScoringAlgorithm() ;

and this at line 639 which is used in the /scoreboard/getStandings() endpoint to obtain the current standings:

          xmlStandings = dsa.getStandings(internalContest, runs, null, props, logger);

followed by this at line 645 which uses the org.json.XML vendor library to convert those XML standings (from the DSA) to corresponding JSON standings:

        currentJSONStandings = this.getJSONStandings(xmlStandings);

(and it is those currentJSONStandings which are returned as the WTI scoreboard values.)

Therefore, I respectfully suggest that the statement WTI uses NSA is wrong.