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

Library override the DPR setting in autonomy without chances to clamp / control it #765

Open
5 tasks done
dghez opened this issue Jul 9, 2024 · 2 comments · Fixed by #768
Open
5 tasks done

Library override the DPR setting in autonomy without chances to clamp / control it #765

dghez opened this issue Jul 9, 2024 · 2 comments · Fixed by #768
Assignees
Labels
feature p2-to-be-discussed Enhancement under consideration (priority)

Comments

@dghez
Copy link

dghez commented Jul 9, 2024

I noticed that Tres set the DPR (see code) completely in autonomy without any way to control that, imho is conceptually wrong for couple of reasons:

  1. The dev should be in charge to decide the DPR or at least be able to clamp. For example if the app is opened on a 3DPR mobile is gonna run at 3, that is completely insane.
  2. The app should not change the dpr on resize without consent. I noticed it because i was setting manually in a range
  const dpr = MathUtils.clamp(window.devicePixelRatio, 1, 1.4) 
 renderer.value.setPixelRatio(dpr)

but on resize switching between monitors on my macbook was set back to 2 due the useDevicePixelRatio hook. This second point tho depends on the first one. If We decide to let the app handle the resize, it's 100% needed to add a clamp on that.

This can lead to some big issues, both perf wise and effect wise. I notice the "issue" because i was doing some effect in screen-space, where my resolution wasn't matching my DPR vec2 st = gl_FragCoord.xy / (gResolution * gDpr);

My take on this is that the library can take care of the DPR but at least give the user some way to control, like they don in r3f where you can set dpr=[low, high] that clamps it :)

Reproduction


Steps to reproduce


System Info

----

Used Package Manager

npm

Code of Conduct

@alvarosabu alvarosabu added feature p2-to-be-discussed Enhancement under consideration (priority) labels Jul 9, 2024
@alvarosabu alvarosabu moved this to Todo in Team Board Jul 10, 2024
@andretchen0 andretchen0 self-assigned this Jul 10, 2024
@andretchen0 andretchen0 moved this from Todo to In Progress in Team Board Jul 10, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Team Board Jul 12, 2024
@dghez
Copy link
Author

dghez commented Jul 23, 2024

hey @alvarosabu /. @andretchen0 saw you took care of this (love) but i think there is still an issue.
Everything works fine because dpr is correctly clamped, although if you try to switch between two monitors with different DPR it changes the DPR of the canvas even without triggering a resize (just move the window between the monitors).

You would think is fine buuut if you are working with screen resolution (so DPR) in a shader this behaviour is kinda odd because you need to update the screen-size there but there is no way to detect is changed without a resize currently.

IMHO solutions are 3

  1. Don't update the DPR without a resize (so you can listen on a resize and update the dpr internally)
  2. Use a ref for the DPR so it can be watched
  3. Just set at the beginning and that's all

@andretchen0
Copy link
Contributor

@dghez

Sure thing. Reopening.

We're currently discussing some changes to the core. I'll make sure that this is part of those discussions.

@andretchen0 andretchen0 reopened this Aug 1, 2024
@andretchen0 andretchen0 moved this from Done to Todo in Team Board Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature p2-to-be-discussed Enhancement under consideration (priority)
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

3 participants