web: Add `forceRenderer` config #10835

relrelb · 2023-04-26T09:40:01Z

Apart from preferredRenderer, which works in a "best effort" manner, is it useful to have a "mandatory" config for testing purposes. That is, if a forced renderer backend fails to initialized, then no further fallback is performed.

n0samu

It is user-hostile to force a panic unnecessarily. I understand that "use a specific renderer or panic" is useful for development purposes. So let's provide an additional forceRenderer boolean that triggers a panic if the chosen preferred renderer cannot be initialized. Don't force extension users and webmasters to do something dangerous just to specify their preferred renderer.

iwannabethedev · 2023-04-26T10:15:51Z

Could it maybe make sense to mimic Phaser 3's similar behavior?

https://newdocs.phaser.io/docs/3.55.2/Phaser.Types.Core.GameConfig

https://newdocs.phaser.io/docs/3.60.0/Phaser.WEBGL

As I understand Phaser 3's behavior, if the setting is "AUTO", Phaser 3 tries WebGL and falls back to Canvas if WebGL is not supported. If the setting is not "AUTO" and instead is specific, like "WEBGL" or "CANVAS", then it does not fall back (maybe fails?).

Phaser 3 and Ruffle have different audiences - Phaser 3 is for game developers only, while Ruffle is for players, webmasters and game/application developers, as I understand Ruffle and Phaser 3. Phaser 3 is also a game engine/framework, while Ruffle is an emulator. And this setting for Phaser is usually set during development of a game, not during runtime by users. But I do imagine that similar behavior as Phaser 3 might make sense. "AUTO" seems to make best sense for players and probably webmasters and game developers, while the specific settings would be useful for players and both game and Ruffle-itself developers trying to debug and play a game that is not working (or test or debug Ruffle).

n0samu · 2023-04-26T10:21:59Z

I'm not open to any change in the design of the preferred renderer feature, sorry. You can add your additional boolean if you like though

iwannabethedev · 2023-04-26T10:25:27Z

I personally like the idea of an additional boolean, and I would not mind looking at providing a PR that implements a boolean that keeps the current behavior while the boolean setting is off and makes the choice strict when the boolean setting is on (with an exception for any "automatic" option, since I think that specific choice should always be flexible). But, I am a minor contributor, and not at all a core developer.

n0samu · 2023-04-26T10:26:24Z

That would be great! Or if relrel wants to update this PR to do that, that would be great as well.

iwannabethedev · 2023-04-26T10:38:04Z

It is user-hostile to force a panic unnecessarily. I understand that "use a specific renderer or panic" is useful for development purposes. So let's provide an additional forceRenderer boolean that triggers a panic if the chosen preferred renderer cannot be initialized. Don't force extension users and webmasters to do something dangerous just to specify their preferred renderer.

After thinking it over, I am warming further up to your argumentation. Comparing with Phaser 3, Phaser 3 has fewer kinds of audiences and use case situations - primarily game developers - and Phaser 3 does provide some flexibility such as in the form of "AUTO". Ruffle has more audiences and use case situations (including runtime situations - possibly primarily runtime situations, given that a very large number of games and applications have already been written), so, from an intuition point of view, providing extra flexibility feels right, at least to me.

torokati44 · 2023-04-26T10:51:00Z

FWIW I'm adding my +1 to the additional "force (no fallback)" boolean/checkbox.

iwannabethedev · 2023-04-26T11:14:49Z

I feel like this is a somewhat complex situation, and I am not the one having to deal with the consequences down the road of whichever choice is made, so I do not feel like I have any right to dictate anything, honestly.

firoball · 2023-04-26T11:15:25Z

View from outside:
Alternatively there could be a "default" list item which keeps the current fallback behaviour, so the common user does not need to bother. But I feel a "force" flag (disabled by default) is the clearer approach.

iwannabethedev · 2023-04-26T18:53:35Z

I wrote some basic use case analysis on Discord.

I feel like there are strong use cases for at least an option like Default/Auto/Automatic, where there is a single setting where Ruffle falls back automatically depending on support, namely an option that users can use without worrying about or having to set the renderer backend used and where it tries to use the best possible renderer. This single setting would start with the "best" possible renderer backend and try down a list to find a renderer backend that is supported in the current environment. That list is currently: ["webgpu", "wgpu-webgl", "webgl", "canvas"], where the first attempted renderer backend is "webgpu". The use would not choose a specific renderer backend, but an abstract "default" or "auto" renderer backend option. We could call this option default.

And there are also clear use cases for having mandatory/forced settings for each renderer backend, namely for Ruffle developers testing a specific renderer backend, or for users, game developers and webmasters for trying out different renderer backends for a given application as part of making that application work as workarounds. We could call these options forced-each.

The use case for having settings similar to Default/Auto/Automatic, but where the first attempted renderer backend is not "webgpu" but set by the user, is less clear to me. As far as I can figure out, the current solution supports this functionality through each option being preferred. I attempted to write some use cases for on Discord. I think if one or more clear use cases for this can be described, then the current solution with having a "preferred" or Default for each renderer backend, together with a forced/mandatory boolean setting, is much more clearly justified. We could call these options preferred-each.

In #9904 , support for preferred-each was implemented. #10831 looks at adding support for default (which with support for preferred-each and #9904 is rather straight-forward). This PR would change the supported functionality from preferred-each to forced-each (and I assume it could also support default with a future PR). Adding a boolean setting in a PR that forces a renderer backend, to the functionality of #9904 and #10831 , would provide support for both preferred-each, default and forced-each.

I believe it is clear-cut that at least forced-each, and also default, should be supported somehow given the strong use cases. (please do correct me if I am wrong on this point).

Issue #9187 describes a real-world use case using the game "Bloons Super Monkey" that supports setting a specific renderer backend by users or webmasters, and this real-world use case supports at least the forced-each options and arguably also the preferred-each options. Issue #7369 is also relevant. VAO issues #1905 and #9665 might also be relevant.

I do not have a good feel for what the drawbacks of supporting preferred-each might be. The discussion in #9904 argues possible API issues. I think one possible take-away from that discussion is that forced-each and preferred-each (excluding default) might be helpfully described to users as an "Advanced" feature, or add a clear notice that any specific renderer backend is not guaranteed to be supported in the future. Such that the feature is not seen as something that users would normally set and that it does not become part of any API or expectation that Ruffle pledges to support. If this clear warning is added (both in the code and also in the UI facing users), like, maybe just "no single specific renderer backend is guaranteed to be supported in the future", is there any drawbacks to including/keeping support for preferred-each?

Finally, I am more or less a newcomer with little insight into Ruffle, so please do correct any and all errors, misconceptions, etc. I might have made in this comment.

relrelb · 2023-04-27T07:10:53Z

Thanks all for your feedback! Sorry for my initial changes, it was I who misunderstood the situation.

Now preferredRenderer is restored and a separate forceRenderer is added. I made preferredRenderer and forceRenderer mutual exclusive (which is enforced by checkOptions in ruffle-player.ts) rather than making forceRenderer a boolean just to clarify things and avoid misconfigurations.

Let me know what do you think.

web/packages/extension/assets/options.html

danielhjacobs · 2023-04-27T13:38:13Z

Change the extension options page to use forceRenderer, since preferredRenderer is only expected to be used by webmasters, not Ruffle developers nor end users.

Isn't it the opposite? End-users should not using an option that can force a panic.

iwannabethedev · 2023-04-29T00:46:06Z

Apart from merging in the most recent changes (including the changes that removed MaxExecutionDurationOption, which I think is a nice change given that MaxExecutionDurationOption was somewhat of an eyesore and rather special-cased, already when I originally wrote it), is there anything missing or needed in this PR? I believe that any "Default" or "Automatic" option, as in #10831 , can always be adapted to the changes of this PR, since this PR deals with more complex issues than #10831 , and so I think merging this first makes sense. I do not mind at all adapting #10831 to this PR after this PR has been merged.

CC @relrelb @n0samu .

n0samu · 2023-04-29T04:10:19Z

Please wait, I need some time to try my own approach, sorry

iwannabethedev · 2023-04-29T04:46:03Z

That sounds good, if you can coordinate with how to approach it, that will be very good. I imagine that two different approaches might create good grounds for taking the best of both. Edit: Hopefully without you spending lots of time and effort on it. I am not certain.

To avoid visual clutter, move some less handy options under a collapsible area: * Log level * Ignore website compatibility warnings * Maximum allowed ActionScript execution duration (description was shortened in this commit)

Apart from `preferredRenderer`, which works in a "best effort" manner, is it useful to have a "mandatory" config for testing purposes. That is, if a forced renderer backend fails to initialize, then no further fallback is performed.

iwannabethedev · 2023-04-30T17:54:45Z

web/packages/extension/assets/options.html

+                            <option value="webgl">WebGL</option>
+                            <option value="canvas">Canvas2D</option>
+                        </select>
+                        <label for="force_renderer">Force renderer</label>


Would it make sense to inform the user somehow of the exclusivity of "Preferred renderer" and "Force renderer", apart from "Preferred renderer" being disabled if "Force renderer" is set?

Like, maybe "Force renderer (exclusive with "Preferred renderer")?

That said, "Force renderer" is an "Advanced option", which helps a lot.

iwannabethedev · 2023-04-30T18:37:28Z

I like the PR overall, there are some nice and very nice parts. There are a few nits (translation, I think). I have some questions, but I think they to some degree depend on the PR in #10831 , which I think will be merged after this one - though, that PR might become just a one-liner given the additions and changes here.

iwannabethedev · 2023-04-30T17:57:00Z

web/packages/extension/assets/options.html

@@ -71,6 +53,37 @@
                />
                <label for="player_version">Flash player version number to emulate (range 1-32)</label>
            </div>
+            <details>
+                <summary>Advanced Options</summary>


Really nice addition with separating certain options into an 'Advanced Options' section.

iwannabethedev · 2023-04-30T18:05:22Z

web/packages/extension/assets/_locales/en/messages.json

@@ -21,7 +21,7 @@
        "message": "Log level"
    },
    "settings_max_execution_duration": {
-        "message": "How long can ActionScript execute for before being forcefully stopped (in seconds)"
+        "message": "Maximum allowed ActionScript execution duration (in seconds)"
    },


Would it make sense to add translation strings for both preferred_renderer and force_renderer? I think it might have been forgotten in #9904 (I forgot it myself in at least one PR until I got reminded of it in a different PR). Unless it was omitted on purpose in #9904 .

From what I remember of the code, it is only necessary to make entries here, since the ID value and for value in the HTML is used for the translation key, so the entry keys would be settings_preferred_renderer and settings_force_renderer .

I didn't know about adding translation strings, so yeah, that was an oversight on my part.

iwannabethedev · 2023-04-30T18:17:23Z

web/packages/extension/src/options.ts

+        "preferred_renderer"
+    )! as HTMLSelectElement;
+    bindOptions((options) => {
+        preferredRenderer.disabled = !!options.forceRenderer;


I am not certain, and this may be a bad idea, but I am wondering if this might make sense:

preferredRenderer.disabled = !!options.forceRenderer && !options.preferredRenderer; forceRenderer.disabled = !!options.preferredRenderer && !options.forceRenderer;

The above is a bit involved, but, is basically meant to ensure that both options disables each other, but also allowing the user to unset one of them if the configuration somehow ends up with both being set.

But will this require a default value for preferredRenderer to be set, which will be added in a different PR as I understand the plans? So it might not make sense to add this now, now that I think about it (if it makes sense in general to add it).

iwannabethedev · 2023-04-30T18:26:20Z

web/packages/core/src/ruffle-player.ts

@@ -644,6 +644,10 @@ export class RufflePlayer extends HTMLElement {
            !("url" in options) || typeof options.url === "string",
            "`url` must be a string"
        );
+        check(


I do not understand the codebase well enough for this, but, will the user see a UI error if they mess up the settings?

Though, it might not be the responsibility of this PR to ensure that the user can see a UI error if that happens, but a new issue and PR.

I am not certain.

Is check meant to be used only for cases where there is a programmer error, or should it also be used for cases where UI-users have made a mistake? If the former, I am not certain that it is a good idea to use check here, unless the UI ensures that a UI-user cannot make a mistake and end up failing here.

Will using the in operator here really work? Should it not instead be:

!(options.preferredRenderer && options.forceRenderer),

n0samu · 2023-05-03T03:36:04Z

Thanks for your work on this! At this point I think each piece of this PR has been either superseded or moved to other PRs, so this one can be closed.

relrelb marked this pull request as ready for review April 26, 2023 09:41

n0samu suggested changes Apr 26, 2023

View reviewed changes

relrelb force-pushed the web_force_renderer branch from fa6abc9 to 4f084cd Compare April 27, 2023 06:55

relrelb changed the title ~~web: Turn preferredRenderer into forceRenderer~~ web: Add forceRenderer config Apr 27, 2023

relrelb force-pushed the web_force_renderer branch from 4f084cd to 36fb2eb Compare April 27, 2023 07:00

n0samu reviewed Apr 27, 2023

View reviewed changes

web/packages/extension/assets/options.html Outdated Show resolved Hide resolved

relrelb force-pushed the web_force_renderer branch 2 times, most recently from 50d497e to c5eb80f Compare April 27, 2023 15:03

relrelb mentioned this pull request Apr 27, 2023

web: Automatically chosen preferred renderer #10831

Closed

relrelb added 2 commits April 30, 2023 20:18

extension: Introduce "Advanced Options"

3f2c820

To avoid visual clutter, move some less handy options under a collapsible area: * Log level * Ignore website compatibility warnings * Maximum allowed ActionScript execution duration (description was shortened in this commit)

web: Add forceRenderer config

1562d4d

Apart from `preferredRenderer`, which works in a "best effort" manner, is it useful to have a "mandatory" config for testing purposes. That is, if a forced renderer backend fails to initialize, then no further fallback is performed.

relrelb force-pushed the web_force_renderer branch from c5eb80f to 1562d4d Compare April 30, 2023 17:24

iwannabethedev reviewed Apr 30, 2023

View reviewed changes

n0samu mentioned this pull request May 1, 2023

web: Various improvements to options #10900

Merged

This was referenced May 1, 2023

extension: Log used renderer; Warn if not preferred. #10903

Closed

extension: Log used renderer. #10909

Merged

n0samu mentioned this pull request May 2, 2023

web: Tiny refactor of preferred renderer code #10921

Merged

relrelb closed this May 3, 2023

relrelb deleted the web_force_renderer branch May 11, 2023 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

web: Add `forceRenderer` config #10835

web: Add `forceRenderer` config #10835

relrelb commented Apr 26, 2023 •

edited

Loading

n0samu left a comment

iwannabethedev commented Apr 26, 2023

n0samu commented Apr 26, 2023

iwannabethedev commented Apr 26, 2023

n0samu commented Apr 26, 2023

iwannabethedev commented Apr 26, 2023

torokati44 commented Apr 26, 2023

iwannabethedev commented Apr 26, 2023

firoball commented Apr 26, 2023

iwannabethedev commented Apr 26, 2023

relrelb commented Apr 27, 2023

danielhjacobs commented Apr 27, 2023

iwannabethedev commented Apr 29, 2023

n0samu commented Apr 29, 2023

iwannabethedev commented Apr 29, 2023 •

edited

Loading

iwannabethedev Apr 30, 2023

iwannabethedev Apr 30, 2023

iwannabethedev commented Apr 30, 2023

iwannabethedev Apr 30, 2023

iwannabethedev Apr 30, 2023

n0samu May 1, 2023

iwannabethedev Apr 30, 2023

iwannabethedev Apr 30, 2023

iwannabethedev Apr 30, 2023

iwannabethedev May 1, 2023

n0samu commented May 3, 2023

web: Add forceRenderer config #10835

web: Add forceRenderer config #10835

Conversation

relrelb commented Apr 26, 2023 • edited Loading

n0samu left a comment

Choose a reason for hiding this comment

iwannabethedev commented Apr 26, 2023

n0samu commented Apr 26, 2023

iwannabethedev commented Apr 26, 2023

n0samu commented Apr 26, 2023

iwannabethedev commented Apr 26, 2023

torokati44 commented Apr 26, 2023

iwannabethedev commented Apr 26, 2023

firoball commented Apr 26, 2023

iwannabethedev commented Apr 26, 2023

relrelb commented Apr 27, 2023

danielhjacobs commented Apr 27, 2023

iwannabethedev commented Apr 29, 2023

n0samu commented Apr 29, 2023

iwannabethedev commented Apr 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iwannabethedev commented Apr 30, 2023

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

n0samu commented May 3, 2023

web: Add `forceRenderer` config #10835

web: Add `forceRenderer` config #10835

relrelb commented Apr 26, 2023 •

edited

Loading

iwannabethedev commented Apr 29, 2023 •

edited

Loading