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

Use max anisotropy value (max. 16) #64

Merged
merged 1 commit into from Dec 5, 2022

Conversation

fulopkovacs
Copy link
Contributor

@fulopkovacs fulopkovacs commented Dec 3, 2022

This PR fixes the blurriness of textures from sharp angles (#45).

I get the maximum valid anisotropy value for the GPU (max. 16) and set it for all the textures in the 3D scene. This places a heavier load on the GPU due to the higher number of texture samples used, but on my machines, I didn't really notice any change in performance.

See the difference in the details on the bottoms of the crates in the screenshot below (diffuse maps only; model: https://polyhaven.com/a/wooden_crate_02):
image

@vercel
Copy link

vercel bot commented Dec 3, 2022

@fulopkovacs is attempting to deploy a commit to the polyhaven Team on Vercel.

A member of the Team first needs to authorize it.

@fulopkovacs fulopkovacs changed the base branch from master to dev December 3, 2022 23:31
@fulopkovacs
Copy link
Contributor Author

There's one thing bothering me about this PR: it seemed to crash on my iPhone, but I'm not sure if it's a problem with the dev setup, my phone, or the PR.

polyhaven.com works fine, but deploys from the dev branch crash when I want to view models. Will check back on it tomorrow or next week.

@fulopkovacs fulopkovacs mentioned this pull request Dec 3, 2022
@gregzaal
Copy link
Member

gregzaal commented Dec 5, 2022

This is awesome, thanks :)

@gregzaal gregzaal merged commit 3c77077 into Poly-Haven:dev Dec 5, 2022
@gregzaal
Copy link
Member

gregzaal commented Dec 5, 2022

I'm pushing to https://dev.polyhaven.com now, should be easier to test there.

@fulopkovacs
Copy link
Contributor Author

@gregzaal Hmmm, I tested it and it crashes on https://dev.polyhaven.com. I did some investigation and this is what I found:

  • ✅ on a 3-yrs-old Samsung (don't know the model) this PR is fine
  • ❌ on my iPhone (iPhone 7 Plus) this PR crashes
  • ✅ on my iPhone (iPhone 7 Plus) the last commit before this PR (ec6c9a7) is fine

After checking the console logs on my iPhone I was able to determine that:

  • the phone reports a max anisotropy value of 16
  • it's okay when I set the anisotropic values only for the solo maps (when only the diffuse map/metalness map/etc is shown)
  • prints no errors regarding this issue in the console
  • it's okay when I set the anisotropic value to the max on the diffuse map's texture when all the maps are displayed

Maybe my phone simply runs out of resources? Or are there limitations on the amount of resources WebGL has access to on iPhones? Not sure.

This issue is a weird one, and I don't really have any ideas on how to proceed with debugging it. Tbh, there's little value in using high anisotropic values on small screens anyway, so I think it would be a good solution to simply avoid changing the anisotropic values on iPhones. What do you think?

Btw, this is the error message that I get when the page crashes:
image

@gregzaal
Copy link
Member

gregzaal commented Dec 5, 2022

I don't have an iphone to test, but can confirm it works on my Mac Air and android phone.

The portion of our users on mobile is <5% last I checked, and the portion of these on iphones is even less, and the portion of those who want to view a 3D model is less still. I'd say it's OK to leave this as-is and address it later if we get more reports (maybe just be reducing/disabling anisotropy if mobile safari is detected).

@fulopkovacs
Copy link
Contributor Author

Okay, considering these stats, I'm okay with this PR being merged! ☺️
(Also, feel free to @ me if people start complaining.)

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.

None yet

2 participants