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

Reconsider default maxCanvasWidth and maxCanvasHeight values #5413

Open
mrxz opened this issue Dec 14, 2023 · 2 comments
Open

Reconsider default maxCanvasWidth and maxCanvasHeight values #5413

mrxz opened this issue Dec 14, 2023 · 2 comments

Comments

@mrxz
Copy link
Contributor

mrxz commented Dec 14, 2023

Description:
By default the renderer resolution is capped at 1920x1920. This was introduced back in 2018 with #3641. There are a couple of downsides to this:

  • Screen resolutions have gone up, making it likely that this cap is hit in everyday configurations
  • Exceeding the cap might go unnoticed, or the resulting blurriness can be difficult to attribute to this cap
  • The width and height take devicePixelRatio into account, which is inconsistent with Three.js ("device pixels" vs "CSS pixels")

I would propose changing the default values to -1 making it uncapped by default. Users can always opt-in to capping the size if they have a specific need for it.
Otherwise it wouldn't hurt log a warning in the console once the cap is exceeded and possible increase the default cap.

  • A-Frame Version: 1.5.0
  • Platform / Device: all
@dmarcos
Copy link
Member

dmarcos commented Dec 14, 2023

Yeah we can revisit the values. In general not super fan of using -1 values to indicate no value. Wonder if it makes sense to keep the limit at all. I personally don't use it and even forgot that it exist :)

@mrxz
Copy link
Contributor Author

mrxz commented Dec 15, 2023

Well, only suggesting -1 since that's already how it works (docs):

if (!maxSize || (maxSize.width === -1 && maxSize.height === -1)) {

Personally I think the limit can be removed entirely. Most people probably don't expect this cap and only learn about it when they or one of their users runs into it. Not to mention that it conflates the canvas size with the render resolution. It's a lot cleaner to have the size be the canvas element size in CSS pixels and use the pixelRatio to adjust the effective render resolutions. That's how I see it done in a lot of Three.js apps nowadays.

That also makes it easier in case users do want to limit it. They can either limit the actual canvas size (when 'embedded') or adjust the renderer's pixelRatio.

If agreed I can create a PR that removes the logic (thus changing the default to not cap the size).

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

No branches or pull requests

2 participants