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

[Community] [New extension] Add poki games sdk extension #840

Merged
merged 7 commits into from
May 16, 2023

Conversation

AlexandreSi
Copy link
Contributor

@AlexandreSi AlexandreSi commented Mar 31, 2023

JS only extension.
The actions names match the ones given in the Poki SDK documentation.

@AlexandreSi AlexandreSi requested a review from a team as a code owner March 31, 2023 14:51
@AlexandreSi
Copy link
Contributor Author

I think the last error "JavaScript disallowed properties" is not relevant here since it's the advised way in https://wiki.gdevelop.io/gdevelop5/extensions/best-practices/#javascript-usage

extensions/community/PokiGamesSDK.json Outdated Show resolved Hide resolved
Comment on lines 45 to 51
"async": true,
"description": "Load Poki SDK.",
"fullName": "Load SDK",
"functionType": "Action",
"group": "Poki Games",
"name": "LoadSDK",
"sentence": "Load Poki SDK",
Copy link
Contributor

@D8H D8H Mar 31, 2023

Choose a reason for hiding this comment

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

Should it be done in a life-cycle function?
and a "is ready" condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We ended up think that it shouldn't be done in the OnFirstSceneLoaded callback to leave the possibility to the user to easily disable the SDK if they want to export the game for a platform that is not Poki. Maybe this logic should be implemented in other platform SDK implementation extensions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding the "Is ready condition", it felt more natural to me to have the action asynchronous. Do you see drawbacks to this?

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if players have an intermittent internet connection and the action fails?
Can the SDK be downloaded as soon as the internet connection comes back?
How do creators know when to try again?
What happens if this action is ran in loop every frame? Is there some kind of protection against flooding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think it's worth adding those checks although this SDK is only useful when the game is played on poki platform that is a website?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not

Copy link
Member

Choose a reason for hiding this comment

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

We ended up think that it shouldn't be done in the OnFirstSceneLoaded callback to leave the possibility to the user to easily disable the SDK if they want to export the game for a platform that is not Poki

I disagree with this, such an sdk should likely be a no-op anyways when it is not ran on poki. If poki's SDK crashes the game or something when it is ran outside poki, then you should add code that checks if the game is running on poki based for example on the URL before loading the SDK.

Asking the user to themselves detect if the game is running on poki is just unnecessary extra work. Not to mention, it is terrible UX to have other actions just no-op by default (or crash the game) if the user didn't know to call an "initialize" action beforehand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were wondering with @HelperWesley what's the best strategy a GDevelop user should use when maintaining a game that is hosted on different platforms (poki, crazy games, etc.). Have you already thought about it? I'm not sure what this strategy would be: I would say: External events and multiple exports, but definitely not loading and removing extensions that work behind the scenes.
I get your point about the URL: do you think I should load the SDL in onFirstSceneLoaded only if the url is under poki.com domain? And po.ki (for their QA tool)?

Copy link
Member

Choose a reason for hiding this comment

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

The game should be working the same on most platforms. The SDKs only provide augmentations, not a different game: e.g. on newgrounds, when a specific action is done you'll unlock a medal, with Poki when losing you'll trigger an ad... Those augmentations are not new game logic, they are just a piece of additional platform-specific logic to trigger during the normal game logic. There is no need to separate codebases! Just call the run ad action when you want an ad to run, unlock medal when you want to unlock a medal... And the extension can simply call the SDK.

If the game is not running in the platform, it should still try if possible to do what it's told: e.g. a newgrounds extensions can still unlock a medal even if you are not running the game from newgrounds, it just opens a login popup instead of auto-logging-in. If the SDK won't allow using it on other sites, e.g. with poki ads, then just create a non-breaking no-op default behavior. The SDK gives the platform a hint to display an ad, if there is no platform then just don't send any hint, don't force the game to run exclusively on that platform!

Copy link
Contributor

Choose a reason for hiding this comment

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

Users could write an extension to abstract the platform-dedicated extensions they use from their games?

Comment on lines 298 to 309
{
"fullName": "",
"functionType": "Action",
"name": "onScenePostEvents",
"sentence": "",
"events": [
{
"type": "BuiltinCommonInstructions::JsCode",
"inlineCode": [
"gdjs._pokiGamesSdkExtension.commercialBreakJustFinishedPlaying = false;",
"gdjs._pokiGamesSdkExtension.rewardedBreakJustFinishedPlaying = false;"
],
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be done in PreEvents in case another extension wants to use the condition in its PostEvents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about that:
Both variables are being set to true when the promise returned by the poki SDK methods resolves. 4ian explained to me that those promises can only be resolved between 2 frames (because of the way JS works - single-threaded-ly), so between the post events of frame N and the pre events of frame N+1.
So these variables would be forever false when checked in the events.
Is it clear or is there something I'm missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see

Copy link
Member

@arthuro555 arthuro555 left a comment

Choose a reason for hiding this comment

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

The extension is missing a few things from the instructions for the HTML5 SDK:

  • It should disable audio while an ad is running, and bring it back once the ad is finished
  • The "Shareable URL" API is missing

extensions/community/PokiGamesSDK.json Outdated Show resolved Hide resolved
Comment on lines 56 to 74
"function addScript(src) {",
" return new Promise((resolve, reject) => {",
" const scriptElement = document.createElement('script');",
"",
" scriptElement.setAttribute('src', src);",
" scriptElement.addEventListener('load', resolve);",
" scriptElement.addEventListener('error', reject);",
"",
" document.body.appendChild(scriptElement);",
" });",
"}",
"addScript('https://game-cdn.poki.com/scripts/v2/poki-sdk.js').then(() => {",
" PokiSDK.init().then(() => {",
" console.log(\"Poki SDK successfully initialized\");",
" gdjs.evtTools.common.resolveAsyncEventsFunction(eventsFunctionContext);",
" }).catch(() => {",
" console.log(\"Initialized, but the user likely has adblock\");",
" });",
"})"
Copy link
Member

Choose a reason for hiding this comment

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

Generally, with JS extensions, we prefer to embed the javascript code in the event rather than load it via a script tag:

  • It does not rely on the DOM (btw, whenever you use a dom api like this, the extension must be tagged as such!)
  • It loads the script immediately allowing to use features directly after
  • It does not rely on an internet connection
  • It does not rely on another server that may be temporarily down
  • If we know a script is safe when creating the extension, we can rest assured it cannot be replaced by a nefarious version by a malicious individual

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll change that

Copy link
Contributor Author

@AlexandreSi AlexandreSi Apr 6, 2023

Choose a reason for hiding this comment

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

I'm not sure I will include the script here entirely: There's a first script that does :

    n = "kids" === e("tag"),
    t = !!window.adBridge,
    o = "yes" === e("gdhoist"),

and then loads different scripts depending on those values:

  var r,
    a,
    u =
      ((r = window.pokiSDKVersion || e("ab") || "v2.338.0"),
      (a = "poki-sdk-core-" + r + ".js"),
      n && (a = "poki-sdk-kids-" + r + ".js"),
      t && (a = "poki-sdk-playground-" + r + ".js"),
      o && (a = "poki-sdk-hoist-" + r + ".js"),
      "https://game-cdn.poki.com/scripts/" + r + "/" + a),
    s = document.createElement("script");
  s.setAttribute("src", u),
    s.setAttribute("type", "text/javascript"),
    s.setAttribute("crossOrigin", "anonymous"),
    (s.onload = function () {
      return i.dequeue();
    }),
    document.head.appendChild(s);

Do you think I should hack the first script to load the right scripts under the right conditions or should I keep the current way because:

  • It does not rely on the DOM (btw, whenever you use a dom api like this, the extension must be tagged as such!)

I'm not sure that's a problem

  • It loads the script immediately allowing to use features directly after

If I add the condition isPokiSdkLoaded, it won't be an issue

  • It does not rely on an internet connection

Poki is an online platform so it's a prerequisite

  • It does not rely on another server that may be temporarily down

I would say it's Poki's problem

  • If we know a script is safe when creating the extension, we can rest assured it cannot be replaced by a nefarious version by a malicious individual

The url seems to used Poki's CDN so I would not be too worried about this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be the tag to use to declare that a DOM api is used?

Copy link
Member

Choose a reason for hiding this comment

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

It does not rely on the DOM (btw, whenever you use a dom api like this, the extension must be tagged as such!)

I'm not sure that's a problem

It is! A game that is published on Poki could also run for example in a server environment if it's a multiplayer game, or a porting company could also run the game on a js runtime with a native rendering library exposed to run the game on a console for example...
Code that expects the DOM APIs to be present will crash for those applications unless you create a separate version of your game without the Poki SDK... Which sucks.

The url seems to used Poki's CDN so I would not be too worried about this

A company getting compromised is not unheward of, and we should definitely be very very careful since we are using electron with all node APIs exposed to the browser, which is a common target for malicious JS code to get a lot of bad things done. This is not just Poki's problem: due to GDevelop's negligence, this would allow all PC builds and the developer's previews to get a lot of people affected.

If I add the condition isPokiSdkLoaded, it won't be an issue

On the contrary, you completely missed the point. Having to wait for the SDK to load with "IsSDKLoaded" is a hassle for us since we must add checks everywhere that the sdk has loaded for the game not to crash if it isn't, for the user that has to make sure not to use the SDK before it is loaded if they don't want it to no-op, create a loading screen waiting for the sdk to load, etc, and also for the player that has to wait extra time for the script to load in the background. It's a bad, error-prone experience for everyone.

and then loads different scripts depending on those values:

Nothing stops you from embedding the different sub-scripts in the js event too :p

extensions/community/PokiGamesSDK.json Outdated Show resolved Hide resolved
extensions/community/PokiGamesSDK.json Outdated Show resolved Hide resolved
{
"type": "BuiltinCommonInstructions::JsCode",
"inlineCode": [
"PokiSDK.gameLoadingFinished();",
Copy link
Member

Choose a reason for hiding this comment

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

If you only conditionally load the Poki SDK, you cannot just call it like this, since it is not guaranteed to exist, and calling a function that does not exist will crash the game. Unless you always load Poki when the first scene loads and do so directly without a DOM script so that the game is guaranteed to run with the Poki SDK loaded at all times, prefix all poki sdk calls with if (typeof PokiSDK !== "undefined") .

@AlexandreSi
Copy link
Contributor Author

  • It should disable audio while an ad is running, and bring it back once the ad is finished

I felt like that's something the creator should handle: Do they want to pause or stop the music or do they want to lower the volume? I don't think this extension should handle this complexity.

  • The "Shareable URL" API is missing

That was not requested by Wesley but I can add it

@AlexandreSi
Copy link
Contributor Author

Ok so, here are the changes from my latest commit:

  • SDK is now loaded in the onFirstSceneLoaded life cycle event
  • There's now a condition to check if the sdk is ready
  • Each instruction calling the sdk returns early if the sdk is not defined
  • I have not added the create shareable url since the poki sdk method returns a promise, I'm not sure how we can handle promises in expressions. Otherwise, it could be an asynchronous action that stores the url into a variable but I'm not sure it's doable.

@AlexandreSi
Copy link
Contributor Author

After a little test, I realized that if the game is run on a no-poki url, the sdk initialization does not fail so the condition isSdkReady will be true.
I could add a second condition is Poki SDK ready to display ads returning a member of the js class PokiSDK.SDK
image

@D8H D8H added ✨ New extension A new extension 👨‍👩‍👧‍👦 Community extension An extension submission to be merged ASAP with a lightweight review. labels Apr 12, 2023
@D8H D8H added this to Needs review in Extensions review via automation Apr 12, 2023
@AlexandreSi
Copy link
Contributor Author

@arthuro555 I must admit that I'm a bit skeptical about all those security and internet condition considerations that should be taken for GDevelop games in this extension even though I only applied what Poki themselves consider what is a correct implementation of their SDK.
Could it help to name this extension "Poki games SDK HTML"?

@github-actions
Copy link
Contributor

Errors were detected in this submission:

❌ 3 Errors found in extension 'PokiGamesSDK':

  ⟶ ❌ (🔧) [Dots in sentences]: Field 'description' of the function 'isSdkReady' misses a dot at the end of the sentence!
  ⟶ ❌ [JavaScript disallowed properties]: Found disallowed properties in extension 'PokiGamesSDK':
{
  allowedProperties: [
    'makeUuid',           'rgbToHex',
    'rgbOrHexToRGBColor', 'rgbToHexNumber',
    'hexNumberToRGB',     'hexToRGBColor',
    'copyArray',          'staticArray',
    'staticArray2',       'staticObject',
    'toDegrees',          'toRad',
    'random',             'randomFloat',
    'randomFloatInRange', 'randomInRange',
    'randomWithStep',     'evtTools',
    'Variable',           'RuntimeObject',
    'Logger'
  ],
  disallowedProperty: '_pokiGamesSdkExtension',
  objectName: 'gdjs'
}
  ⟶ ❌ [PascalCase for internals names]: Internal name 'isSdkReady' should begin with an uppercase letter (IsSdkReady)!


❌ 3 Errors found in extensions - please fix them before generating the registry.

@github-actions
Copy link
Contributor

Errors were detected in this submission:

❌ 4 Errors found in extension 'PokiGamesSDKHtml':

  ⟶ ❌ (🔧) [Dots in sentences]: Field 'description' of the function 'isSdkReady' misses a dot at the end of the sentence!
  ⟶ ❌ [JavaScript disallowed properties]: Found disallowed properties in extension 'PokiGamesSDKHtml':
{
  allowedProperties: [
    'makeUuid',           'rgbToHex',
    'rgbOrHexToRGBColor', 'rgbToHexNumber',
    'hexNumberToRGB',     'hexToRGBColor',
    'copyArray',          'staticArray',
    'staticArray2',       'staticObject',
    'toDegrees',          'toRad',
    'random',             'randomFloat',
    'randomFloatInRange', 'randomInRange',
    'randomWithStep',     'evtTools',
    'Variable',           'RuntimeObject',
    'Logger'
  ],
  disallowedProperty: '_pokiGamesSdkExtension',
  objectName: 'gdjs'
}
  ⟶ ❌ [Extension name consistency]: Extension filename should be exactly the name of the extension (with .json extension). Please rename 'PokiGamesSDK.json' to 'PokiGamesSDKHtml.json'.
  ⟶ ❌ [PascalCase for internals names]: Internal name 'isSdkReady' should begin with an uppercase letter (IsSdkReady)!


❌ 4 Errors found in extensions - please fix them before generating the registry.

@github-actions
Copy link
Contributor

Errors were detected in this submission:

❌ 2 Errors found in extension 'PokiGamesSDKHtml':

  ⟶ ❌ [JavaScript disallowed properties]: Found disallowed properties in extension 'PokiGamesSDKHtml':
{
  allowedProperties: [
    'makeUuid',           'rgbToHex',
    'rgbOrHexToRGBColor', 'rgbToHexNumber',
    'hexNumberToRGB',     'hexToRGBColor',
    'copyArray',          'staticArray',
    'staticArray2',       'staticObject',
    'toDegrees',          'toRad',
    'random',             'randomFloat',
    'randomFloatInRange', 'randomInRange',
    'randomWithStep',     'evtTools',
    'Variable',           'RuntimeObject',
    'Logger'
  ],
  disallowedProperty: '_pokiGamesSDKHtmlExtension',
  objectName: 'gdjs'
}
  ⟶ ❌ [Extension name consistency]: Extension filename should be exactly the name of the extension (with .json extension). Please rename 'PokiGamesSDK.json' to 'PokiGamesSDKHtml.json'.


❌ 2 Errors found in extensions - please fix them before generating the registry.

@github-actions
Copy link
Contributor

Errors were detected in this submission:

❌ 1 Error found in extension 'PokiGamesSDKHtml':

  ⟶ ❌ [JavaScript disallowed properties]: Found disallowed properties in extension 'PokiGamesSDKHtml':
{
  allowedProperties: [
    'makeUuid',           'rgbToHex',
    'rgbOrHexToRGBColor', 'rgbToHexNumber',
    'hexNumberToRGB',     'hexToRGBColor',
    'copyArray',          'staticArray',
    'staticArray2',       'staticObject',
    'toDegrees',          'toRad',
    'random',             'randomFloat',
    'randomFloatInRange', 'randomInRange',
    'randomWithStep',     'evtTools',
    'Variable',           'RuntimeObject',
    'Logger'
  ],
  disallowedProperty: '_pokiGamesSDKHtmlExtension',
  objectName: 'gdjs'
}


❌ 1 Error found in extensions - please fix it before generating the registry.

@AlexandreSi
Copy link
Contributor Author

I pushed a few changes. I added the HTML tag to the extension. I can add some warnings if you'd like but it seems fine to me as is. Wesley will test it on monday or the following days to make sure it passes the poki QA tests

@AlexandreSi AlexandreSi merged commit 07ef40b into main May 16, 2023
Extensions review automation moved this from Needs review to Added to GDevelop May 16, 2023
@AlexandreSi AlexandreSi deleted the add-poki-sdk-extension branch May 16, 2023 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👨‍👩‍👧‍👦 Community extension An extension submission to be merged ASAP with a lightweight review. ✨ New extension A new extension
Projects
Extensions review
  
Added to GDevelop
Development

Successfully merging this pull request may close these issues.

None yet

3 participants