-
Notifications
You must be signed in to change notification settings - Fork 17
Better commenting and unify code formatting #49
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
base: main
Are you sure you want to change the base?
Changes from all commits
6460873
b383729
3ae3b6b
036b931
2a9930b
e07cb98
c5d78f1
35ac94e
1443b3d
e75c266
a35d216
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't add a comment on the exact lines, but export class LoadingOverlay extends TextOverlay { Above the class definition, I think we should include some commenting explaining what the class is, it's basic function, core functionality, etc |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ export class LoadingOverlay extends TextOverlay { | |
|
||
private static _rootElement: HTMLElement; | ||
private static _textElement: HTMLElement; | ||
private static _spinner: HTMLElement; | ||
private static _spinnerElement: HTMLElement; | ||
|
||
/** | ||
* @returns The created root element of this overlay. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could further expand commenting here to explain 'what is the root element' and why it's needed, what its purpose is etc. (just to make it obvious) |
||
|
@@ -31,27 +31,27 @@ export class LoadingOverlay extends TextOverlay { | |
|
||
|
||
public static spinner(): HTMLElement { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could use some commenting explaining what this function does |
||
if (!LoadingOverlay._spinner) { | ||
if (!LoadingOverlay._spinnerElement) { | ||
// build the spinner div | ||
const size = LoadingOverlay._rootElement.clientWidth * 0.03; | ||
LoadingOverlay._spinner = document.createElement('div'); | ||
LoadingOverlay._spinner.id = "loading-spinner" | ||
LoadingOverlay._spinner.className = "loading-spinner"; | ||
LoadingOverlay._spinner.setAttribute("style", "width: " + size + "px; height: " + size + "px;"); | ||
LoadingOverlay._spinnerElement = document.createElement('div'); | ||
LoadingOverlay._spinnerElement.id = "loading-spinner" | ||
LoadingOverlay._spinnerElement.className = "loading-spinner"; | ||
LoadingOverlay._spinnerElement.setAttribute("style", "width: " + size + "px; height: " + size + "px;"); | ||
|
||
const SpinnerSectionOne = document.createElement("div"); | ||
SpinnerSectionOne.setAttribute("style", "width: " + size * 0.8 + "px; height: " + size * 0.8 + "px; border-width: " + size * 0.125 + "px;"); | ||
LoadingOverlay._spinner.appendChild(SpinnerSectionOne); | ||
LoadingOverlay._spinnerElement.appendChild(SpinnerSectionOne); | ||
|
||
const SpinnerSectionTwo = document.createElement("div"); | ||
SpinnerSectionTwo.setAttribute("style", "width: " + size * 0.8 + "px; height: " + size * 0.8 + "px; border-width: " + size * 0.125 + "px;"); | ||
LoadingOverlay._spinner.appendChild(SpinnerSectionTwo); | ||
LoadingOverlay._spinnerElement.appendChild(SpinnerSectionTwo); | ||
|
||
const SpinnerSectionThree = document.createElement("div"); | ||
SpinnerSectionThree.setAttribute("style", "width: " + size * 0.8 + "px; height: " + size * 0.8 + "px; border-width: " + size * 0.125 + "px;"); | ||
LoadingOverlay._spinner.appendChild(SpinnerSectionThree); | ||
LoadingOverlay._spinnerElement.appendChild(SpinnerSectionThree); | ||
Comment on lines
42
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is |
||
} | ||
return LoadingOverlay._spinner; | ||
return LoadingOverlay._spinnerElement; | ||
} | ||
/** | ||
* Construct a connect overlay with a connection button. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This says construct a connect overlay yet we call the class LoadingOverlay. This doesn't seem relevant |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,32 +1,45 @@ | ||
import { Application, SettingUIFlag, UIOptions } from '@epicgames-ps/lib-pixelstreamingfrontend-ui-ue5.4'; | ||
import { AggregatedStats, SettingFlag, TextParameters } from '@epicgames-ps/lib-pixelstreamingfrontend-ue5.4'; | ||
import { LoadingOverlay } from './LoadingOverlay'; | ||
import { SPSSignalling } from './SignallingExtension'; | ||
import { MessageStats } from './Messages'; | ||
import { | ||
AggregatedStats, | ||
SettingFlag, | ||
TextParameters | ||
} from '@epicgames-ps/lib-pixelstreamingfrontend-ue5.4'; | ||
import { | ||
Application, | ||
SettingUIFlag, | ||
UIOptions | ||
} from '@epicgames-ps/lib-pixelstreamingfrontend-ui-ue5.4'; | ||
|
||
// For local testing. Declare a websocket URL that can be imported via a .env file that will override | ||
// the signalling server URL builder. | ||
declare var WEBSOCKET_URL: string; | ||
|
||
|
||
export class SPSApplication extends Application { | ||
private loadingOverlay: LoadingOverlay; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is |
||
private signallingExtension: SPSSignalling; | ||
|
||
// Create a flags class for the send stats to server | ||
static Flags = class { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this can use some more explanation (perhaps why we do this and even a link to how PSInfra docs on how we know how to add custom stats). |
||
static sendToServer = "sendStatsToServer" | ||
} | ||
|
||
constructor(config: UIOptions) { | ||
super(config); | ||
|
||
// Initialise the signaling server extensions to support sps signalling messages | ||
this.signallingExtension = new SPSSignalling(this.stream.webSocketController); | ||
this.signallingExtension.onAuthenticationResponse = this.handleSignallingResponse.bind(this); | ||
this.signallingExtension.onInstanceStateChanged = this.handleSignallingResponse.bind(this); | ||
|
||
// Enforce the munging of the websocket address to support SPS | ||
this.enforceSpecialSignallingServerUrl(); | ||
|
||
// Add 'Send Stats to Server' checkbox | ||
// Add a SPS settings section to the settings panel | ||
const spsSettingsSection = this.configUI.buildSectionWithHeading(this.settingsPanel.settingsContentElement, "Scalable Pixel Streaming"); | ||
|
||
// Add the 'Send Stats to server' check box to the list of settings | ||
const sendStatsToServerSetting = new SettingFlag( | ||
SPSApplication.Flags.sendToServer, | ||
"Send stats to server", | ||
|
@@ -35,9 +48,13 @@ export class SPSApplication extends Application { | |
this.stream.config.useUrlParams | ||
); | ||
|
||
// Add the 'Send Stats to server' check box to the sps settings section | ||
spsSettingsSection.appendChild(new SettingUIFlag(sendStatsToServerSetting).rootElement); | ||
|
||
// Initialise the loading overlay | ||
this.loadingOverlay = new LoadingOverlay(this.stream.videoElementParent); | ||
|
||
// Add an event handler for when the statReceive event is emitted | ||
this.stream.addEventListener( | ||
'statsReceived', | ||
({ data: { aggregatedStats } }) => { | ||
|
@@ -48,38 +65,68 @@ export class SPSApplication extends Application { | |
); | ||
} | ||
|
||
/** | ||
* Handled the response when the sps messages are emitted | ||
* @param signallingResp the informative response message | ||
* @param isError if the message is an error | ||
*/ | ||
handleSignallingResponse(signallingResp: string, isError: boolean) { | ||
|
||
// Check if the message is an error | ||
if (isError) { | ||
|
||
// Show the error overlay | ||
this.showErrorOverlay(signallingResp); | ||
} else { | ||
|
||
// Show the loading overlay | ||
this.showLoadingOverlay(signallingResp); | ||
} | ||
} | ||
|
||
/** | ||
* Enforces changes the websocket path to conform to the SPS specification | ||
* Can be overridden if the WEBSOCKET_URL var defined in the .env file has been defined | ||
*/ | ||
enforceSpecialSignallingServerUrl() { | ||
|
||
// SPS needs to build a specific signalling server url based on the application name so K8s can distinguish it | ||
this.stream.setSignallingUrlBuilder(() => { | ||
|
||
// if we have overriden the signalling server URL with a .env file use it here | ||
if (WEBSOCKET_URL !== undefined ) { | ||
// Check if the WEBSOCKET_URL var in the .env file has been defined | ||
if (WEBSOCKET_URL !== undefined) { | ||
|
||
// Return the value of the WEBSOCKET_URL var defined in the .env file | ||
return WEBSOCKET_URL as string; | ||
} | ||
|
||
// get the current signalling url | ||
// Get the current signalling server URL from the settings | ||
let signallingUrl = this.stream.config.getTextSettingValue(TextParameters.SignallingServerUrl); | ||
|
||
// build the signalling URL based on the existing window location, the result should be 'domain.com/signalling/app-name' | ||
// Build the signalling URL based on the existing window location, the result should be 'domain.com/signalling/app-name' | ||
signallingUrl = signallingUrl.endsWith("/") ? signallingUrl + "signalling" + window.location.pathname : signallingUrl + "/signalling" + window.location.pathname; | ||
|
||
// Return the modified signalling server URL | ||
return signallingUrl | ||
}); | ||
} | ||
|
||
showLoadingOverlay(signallingResp: string) { | ||
/** | ||
* Shows the loading overlay | ||
* @param message The message to display | ||
*/ | ||
showLoadingOverlay(message: string) { | ||
|
||
// Hide the current overlay | ||
this.hideCurrentOverlay(); | ||
|
||
// Show the loading overlay | ||
this.loadingOverlay.show(); | ||
this.loadingOverlay.update(signallingResp); | ||
|
||
// Update the loading overlay with the signalling response | ||
this.loadingOverlay.update(message); | ||
|
||
// Set the current overlay to the loading overlay | ||
this.currentOverlay = this.loadingOverlay; | ||
} | ||
|
||
|
@@ -88,7 +135,11 @@ export class SPSApplication extends Application { | |
* @param stats - Aggregated Stats | ||
*/ | ||
sendStatsToSignallingServer(stats: AggregatedStats) { | ||
|
||
// Create a new stats signalling message | ||
const data = new MessageStats(stats); | ||
|
||
// Send the stats message to the signalling server | ||
this.stream.webSocketController.webSocket.send(data.payload()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newline missing at EOF