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

Fixes! #30

Merged
merged 3 commits into from
Jan 19, 2024
Merged

Fixes! #30

merged 3 commits into from
Jan 19, 2024

Conversation

momintlh
Copy link
Collaborator

@momintlh momintlh commented Jan 16, 2024

  • Tested by Playroom team
  • Tested by users
  • Tested by Playroom team
  • Tested by users
  • Tested by Playroom team
  • Tested by users
  • Tested by Playroom team
  • Tested by Playroom team
  • Tested by users

Did a build test on the demo game.

@momintlh momintlh requested review from asadm and SaadBazaz January 16, 2024 14:32
@SaadBazaz
Copy link
Collaborator

Please share a test report. We can't afford feature degradation rn.

Copy link
Collaborator

@SaadBazaz SaadBazaz left a comment

Choose a reason for hiding this comment

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

Approved, with suggestions.

@@ -23,7 +24,6 @@ mergeInto(LibraryManager.library, {
}

var options = optionsJson ? JSON.parse(UTF8ToString(optionsJson)) : {};
// console.log(options)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove, don't comment.
Keeps the code neat and clean.

@@ -929,7 +929,7 @@ public void SetState(string key, float value, bool reliable = false)
{
if (IsRunningInBrowser())
{
SetPlayerStateFloatByPlayerId(id, key, value.ToString(), reliable);
SetPlayerStateFloatByPlayerId(id, key, value.ToString(CultureInfo.InvariantCulture), reliable);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this done everywhere where ToString is used? If yes, has it been tested to not have any negative effects?

@@ -12,6 +12,7 @@ mergeInto(LibraryManager.library, {
// onLaunchCallback
) {
function embedScript(src) {
script.crossOrigin = 'anonymous';
Copy link
Collaborator

Choose a reason for hiding this comment

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

tested this? does this cause any issues?

Copy link
Collaborator

@SaadBazaz SaadBazaz Jan 19, 2024

Choose a reason for hiding this comment

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

Requested and tested by a user here #25.

Also tested locally. Does not break the SDK in any way, however, the security of it is debatable. We shouldn't allow all origins I suppose. @momintlh

Will keep this under watch.

@momintlh momintlh merged commit f445633 into main Jan 19, 2024
2 checks passed
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.

Input on lobby not working in sample project
3 participants