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

web: Add "preferred renderer" config option #9904

Merged
merged 2 commits into from
Apr 25, 2023

Conversation

n0samu
Copy link
Member

@n0samu n0samu commented Mar 7, 2023

This allows users of web builds of Ruffle to choose a preferred render backend. Ruffle will prefer the specified backend while retaining the same fallback order as before. Includes a dropdown on the extension settings page.
image

Implements the feature described in and closes #9187.

@n0samu n0samu force-pushed the renderer-config branch from 8a3c364 to 98f33b0 Compare March 7, 2023 12:33
@Dinnerbone
Copy link
Contributor

Dinnerbone commented Mar 7, 2023

So we've discussed this a bit before but I'll write down my concerns here:

  • WebGL has no regression tests and its infrequency of use makes it possible to break going forwards.
  • The reason I've heard for wanting to use WebGL over WGPU is that it's faster to startup, but that's only true right now and hopefully WGPU-Webgl will be faster in the future. If we decide to support WebGL and add more support or optimizations to it, it definitely won't be as fast as it is today.
  • By adding this feature we are guaranteeing that WebGL (and for that matter, wgpu-webgl) sticks around for the future, as it's now part of our API. Our API can only be additive, really.
  • We will need to decide what to do about bugs and missing features to WebGL, it's either going to be classed as a supported backend that should receive features or it's a fallback-only like canvas. Today it only runs for people who have WebGL1 and not 2, so if we treat it as a fallback we can remove WebGL2 from it and focus instead on making sure it runs the best it can on WebGL1.

If we don't decide to support webgl as a major backend, how do you explain this to the user? "webgl" vs "wgpu" doesn't explain anything. Maybe they'd have heard of webgl, they wouldn't have heard of wgpu. They'd need to know to expect things not working and that we're not going to fix it. The same can be said for canvas, too.
If we do decide to support webgl then we should reconsider having wgpu-webgl as it honestly doesn't make much sense to have two precisely duplicated backends, more things to support and more work when doing anything that touches the renderer. We should get the regression tests running on webgl backend, catch it up to where wgpu-webgl is, and remove wgpu-webgl from the build.

In either scenario, having a choice doesn't make sense to me. If it's not supported, I don't understand any possible case for users intentionally choosing it - and if it is supported, it should be the only supported option.

For this specific PR: if we were to do this, it needs to be a priority list, where today it'd be something like "wgpu-webgpu, wgpu-webgl, webgl, canvas". Say a web host chose "webgpu" - suddenly nobody's going to be able to access Ruffle because that's not working for the vast majority of people today. You shouldn't be picking one renderer, you should be picking the preferred order.

Edit: To be clear, I'm not really advocating for webgl vs wgpu-webgl sticking around here. I personally don't care about the technology we use, other than it'll suck to have to undo and redo all that work I've put in as the original hope was to switch to one common supported backend. If that's what we decide then that's it and I'll just get to work. I just don't want both, it doesn't make sense at all.

@SN902
Copy link

SN902 commented Mar 7, 2023

I think we should focus on WGPU, it will be faster in the future for most users with GTX 960 or better. If we fallback to old realization, it will be minor technical issue for most people with good hardware (as example how x86 applications slowdown performance at 5-10% and make trouble with memory barriers, they needed to patch with PAE to take advantage of 4GB RAM). Why 95% of people must shoot in own legs and helping 5% worldwide users with very outdated PC's, as i seen in some bug reports, they have GTX 650, 7xx or past gen intel integrated GPU's. Even if we stick on WebGL1, their systems may struggle with bad performance on some very complicated swf animations.

@torokati44
Copy link
Member

While development, this could make it easier to test, say, the Canvas2D backend. But there are ways to force either backend to be used on a dev machine. Otherwise I mostly agree with @Dinnerbone.
Also there's an issue where we can't have both the WebGPU and WebGL back-backends of the wgpu backend enabled in any single build AFAIK. - So unless we make a separate, direct WebGPU backend (skipping wgpu) - which I'm not sure why we would do -, those two options can't really coexist. ... (Unless the extension and a selfhosted build are configured with one and the other, but that's way too much of a hypothetical.)

@Dinnerbone
Copy link
Contributor

Also there's an issue where we can't have both the WebGPU and WebGL back-backends of the wgpu backend enabled in any single build AFAIK. - So unless we make a separate, direct WebGPU backend (skipping wgpu) - which I'm not sure why we would do -, those two options can't really coexist. ... (Unless the extension and a selfhosted build are configured with one and the other, but that's way too much of a hypothetical.)

WGPU are working on that, and we can workaround that by hypothetically splitting off each renderer into its own build, switched the same way we do vanilla vs extensions wasm

@adrian17
Copy link
Collaborator

adrian17 commented Mar 7, 2023

I think we should focus on WGPU, it will be faster in the future for most users with GTX 960 or better.

Not sure what you mean here; wgpu-webgl currently is definitely much heavier on low-spec devices (and in principle, wgpu-webgl can never be lighter than pure webgl). I cry inside every time I open https://ruffle.rs/demo/ in my VM to do any manual experimenting :/

(note: that doesn't mean anything about how future WebGPU would work)

@n0samu
Copy link
Member Author

n0samu commented Mar 8, 2023

By adding this feature we are guaranteeing that WebGL (and for that matter, wgpu-webgl) sticks around for the future, as it's now part of our API. Our API can only be additive, really.

No, this PR adds a preferred renderer config option. Notice that the default value is actually webgpu, which is not actually available at the moment, and thus is never used. We aren't guaranteeing that Ruffle will honor the user's preference. We can still remove or change our list of renderers at any time, and we should make that clear in our documentation. (I can add a blurb about that in the doc comment as well if you like.)

They'd need to know to expect things not working and that we're not going to fix it. The same can be said for canvas, too.

You're right. Although right now, using the Canvas backend is a way to avoid the stroke width issue and thus for some specific content, the Canvas backend is the best choice. See #9187 (which I forgot to link to this PR initially). Anyway, we can explain on our wiki page that wgpu is the only actively developed and supported backend at the moment.

I don't understand any possible case for users intentionally choosing it

Please do take a look at #9187.

Say a web host chose "webgpu" - suddenly nobody's going to be able to access Ruffle because that's not working for the vast majority of people today.

What? Please test out my PR, that's not how it works at all!

@Herschel
Copy link
Member

Herschel commented Mar 15, 2023

This is more of a developer/power-user feature, not a public-intended API; so I don't think API concerns really apply here. I've wanted this for quite so time, mainly because it's such a hassle to test specific backends. For example, this allows us to make the renderer selectable in the demo page, or use a specific backend when I'm testing dummy sites on a local web server.

  • Naturally the API should be documented appropriately: "unstable API; renderers are subject to change and may be added, removed, and broken; use at your own risk". We could also just make it @internal, but I don't think being public is a problem as long as the behavior is documented.
  • The behavior for an invalid renderer can simply fallback to the best available renderer, and this can be mentioned in the docs. A priority list is unnecessary.
  • Maybe use a string instead of enum for the renderer type to indicate non-exhaustiveness? Maybe that's overkill, not sure what the idiomatic TypeScript is.
  • I will love not having toggle on/off hardware acceleration when I want to test the canvas backend. 😄

@Herschel
Copy link
Member

  • Maybe use a string instead of enum for the renderer type to indicate non-exhaustiveness? Maybe that's overkill, not sure what the idiomatic TypeScript is.

Thinking more about this, an enum is fine -- I think it's okay for us to codify which backends exist in the API without guaranteeing anything about their actual support or behavior, as long as this is documented. Removing a backend should be considered a semver breaking change, anyway.

@Toad06
Copy link
Member

Toad06 commented Mar 17, 2023

Thanks for your work on this. I think this option is definitely useful for testing and could also be used as a workaround for users facing the VAO issues (#1905 / #9665).

I tried the PR, it works well with the self hosted build and also with the extension when the SWF is open directly. However, the option seems to be ignored when the SWF is played inside a webpage with the extension - maybe that the option needs to be added here:

await sendMessageToPage({
type: "load",
config: {
warnOnUnsupportedContent: options.warnOnUnsupportedContent,
logLevel: options.logLevel,
showSwfDownload: options.showSwfDownload,
},
});
})();

I also think it could be worth to mention that this is quite an advanced option, to discourage users from changing it if they don't understand what this option is meant to be. Something like that for instance:
options

@n0samu
Copy link
Member Author

n0samu commented Mar 18, 2023

@Toad06 Thanks so much for pointing out that issue, I completely missed it. It should be fixed now!

As for your suggestion, I decided to rearrange the extension options from most to least commonly used, and by type:
image

I also removed the "Ignore website compatibility warnings" option from the extension popup since it is not very useful or commonly used.

@n0samu n0samu force-pushed the renderer-config branch from 1a88480 to 87ac412 Compare April 6, 2023 01:14
@Herschel
Copy link
Member

Herschel commented Apr 9, 2023

I think the "wgpu" label should be either wgpu (WebGL) or WebGL (wgpu). Otherwise this looks great to me.

Thanks! I'd like to get this in sooner than later so that we can integrate this before WebGPU goes live in Chrome 113. That way we can play with the different renderers more easily on the demo page.

@danielhjacobs Would you have any interest in adding a selector for this on the demo page? I guess to start this could be an additional dropdown somewhere, or maybe we turn the ℹ️ panel into a full options panel.

@n0samu n0samu force-pushed the renderer-config branch from 87ac412 to d9750a4 Compare April 9, 2023 21:43
@n0samu
Copy link
Member Author

n0samu commented Apr 9, 2023

Alright, I've changed the dropdown item on the extension options page to say "Wgpu (via WebGL)".

@Dinnerbone Can you take a quick look at the doc comments I wrote and let me know if you think they're good?

@n0samu
Copy link
Member Author

n0samu commented Apr 10, 2023

As for the demo page, I think a renderer dropdown would be great to add to Ruffle-Regressions. Considering its limited usefulness to most users, we may not want to add it to the official demo though.

web/packages/core/src/load-options.ts Outdated Show resolved Hide resolved
web/packages/core/src/load-options.ts Outdated Show resolved Hide resolved
@n0samu n0samu force-pushed the renderer-config branch 2 times, most recently from a7f7b7e to 4230e32 Compare April 24, 2023 02:11
Copy link
Contributor

@relrelb relrelb left a comment

Choose a reason for hiding this comment

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

@n0samu Sorry about conflicting with #10814. This PR looks good, I think it can be merged once rebased.

@n0samu
Copy link
Member Author

n0samu commented Apr 25, 2023

Okay, rebased

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.

web: Add 'preferred renderer' config option
10 participants