google / model-viewer

Easily display interactive 3D models on the web and in AR!
https://modelviewer.dev
Apache License 2.0
6.97k stars 823 forks source link

Support higher resolution skyboxes (Background image quality is very low) #912

Closed suro7 closed 4 years ago

suro7 commented 4 years ago

Description

I added the high quality equirectangular image as a background but the model viewer is showing it in very low quality Here's the link to image file: https://3dfascination.com/3d-scamera-web/ar/sphere.jpg

Live Demo

https://3dfascination.com/3d-scamera-web/ar/background.html

cdata commented 4 years ago

Thanks for filing this @suro7 . This was a compromise we made for the sake of performance (mainly memory overhead). At the time, we also filed https://github.com/GoogleWebComponents/model-viewer/issues/704 as a potential follow-on task to make it possible to configure the "blur" to your liking.

The main reason we felt justified in making this trade-off is that it seemed like most of our users are not actually using the skybox. But, if folks think this is important then we can increase the priority of this work.

cc @elalish

elalish commented 4 years ago

Specifically, we use a 256x256 cubemap, which means a 1k (1024x512) equirectangular image is best (it will actually look somewhat better than your high-res one, as the interpolation will be correct). We settled on this to encourage bandwidth reduction. If high-res skyboxes are important, we can look into supporting them. Nice 3D scan, by the way!

suro7 commented 4 years ago

Hi guys, thanks for feedback. Would be great if there will be a possibility to support settings for background quality.

suro7 commented 4 years ago

Specifically, we use a 256x256 cubemap, which means a 1k (1024x512) equirectangular image is best (it will actually look somewhat better than your high-res one, as the interpolation will be correct). We settled on this to encourage bandwidth reduction. If high-res skyboxes are important, we can look into supporting them. Nice 3D scan, by the way!

Thanks! You can have a look on our 3D scanning technologies here: https://www.3dfascination.com/

suro7 commented 4 years ago

Hi again. @elalish Would it be possible to get model-viewer.js file without any background image file resolution limitations?

cdata commented 4 years ago

@suro7 you are certainly free to hack on <model-viewer> yourself. We made the project open source so that you can do that. Contributions are also welcome in the form of PRs. I'm happy to answer any questions you have if you need help getting set up to build the project for yourself.

I think there is a good chance we will make this configurable eventually, but it's mostly a question of priority for us. If there is a groundswell of interest from the community, then we will probably do it sooner, but in the mean time there are other things we need to get done first.

@elalish would you consider #704 a good proxy for the feature being requested here, or should we file a different issue?

elalish commented 4 years ago

@suro7 We've been recently upstreaming some changes to three.js; once those come down it should be easier for us to add this feature.

@cdata No, I think this issue is better (it's not related to blur). Maybe retitle it to "Support higher resolution skyboxes".

suro7 commented 4 years ago

@cdata any suggestions for "hacking"? I think it should be quite easy to disable the skybox decimation or? But it's hard to find out where exactly we should do code changes from such a large source code :)

cdata commented 4 years ago

@suro7 I would start here: https://github.com/GoogleWebComponents/model-viewer/blob/00d7b9d119b7ac9147161f88f17a874cf369b5e1/src/features/environment.ts#L111-L179

This is the part of the code where we generate the skybox and the environment lighting for the model based on attribute/property changes (e.g., modelViewer.backgroundImage = 'foo.hdr').

You'll quickly arrive here: https://github.com/GoogleWebComponents/model-viewer/blob/00d7b9d119b7ac9147161f88f17a874cf369b5e1/src/three-components/TextureUtils.ts#L124-L170

One easy (but possibly expensive) hack would be to change the value returned by that method so that it returns the skybox of your choice (which could be at any resolution you choose).

suro7 commented 4 years ago

@cdata thanks for information, but it's still very hard to distinguish a part just to "disable" decimation feature or to configure to specific resolution. I guess it should be easy to suggest a part where cubebox resolution is defined? can you please suggest where can I simply change it to 4000px*2000px equirectangular photo resolution? We would really appreciate your hint! Thanks in advance

elalish commented 4 years ago

@suro7 Unfortunately it's not quite as simple as you're hoping. The reason is that currently to keep our memory footprint down we reuse the environment sampling texture for the skybox. The environment texture is a custom format that is currently hard-coded to a resolution of 256x256. In order to get a higher resolution skybox, you'll need to fork this project to give the skybox a completely different texture, as well as change our skybox code to use that texture format. This should get a bit easier when we upgrade to the forthcoming r112 version of three.js, at which point skyboxes will be handled in a less custom way.

suro7 commented 4 years ago

@elalish thanks for fast reply. Would it be possible just to decrease skybox resolution to 1024x1024? where can I find appropriate code snippet for it? another question which is critical for us - do you have some time estimation of when upgrade will be made?

suro7 commented 4 years ago

@elalish we do not care about memory footprint at all at the moment. we just want to have better background image, because at the moment it's completely useless..

elalish commented 4 years ago

@suro7 I would strongly recommend for now just resizing your skybox texture to 1024x512 in any image editor and uploading that. Large skybox textures often take longer to fetch than the whole 3D model, so they have not been our focus so far. I can't promise when this will change, but I appreciate you putting it on our radar. I hope the r112 upgrade will happen within a month or two, and at that point it would be easier for you to contribute this change.

suro7 commented 4 years ago

@elalish it seems three.js is on r155. would it be possible to implement higher resolution skyboxes already?

elalish commented 4 years ago

@suro7 Indeed, thanks for the reminder. Yes, this should be easier now and the place you'll want to edit is here. Instead of using PMREMGenerator to make a cubeUV for the skybox, you'll want to just load it as a regular cubemap of your desired resolution, which Three.js has functions for. I believe the rest should just work now. You can just skip the caching; that's the main reason we hadn't implemented something like this yet. Still, if you come up with a good general solution, we'd love to see a PR for it!

suro7 commented 4 years ago

@elalish, @soraryu can you please give some hints or code snippets on what I should change there? I'm not familiar with Three.js funcitons and PMREMGenerator or cubemap implementations.

suro7 commented 4 years ago

hi @cdata @elalish @soraryu Do you have any suggestions on this?

ozgurduygu commented 4 years ago

I'm having issues with this as well. #704 sounds like a good idea, any plans implementing it soon?

elalish commented 4 years ago

@ozgurduygu We're a bit shorthanded at the moment, but I'd be happy to review a PR if you'd like to contribute! See my link above on where you'll want to make the change.

antekj commented 4 years ago

Hi, any chance that there is a quick (even hacky) way of maintaining the full resolution of the environment map? My coding skills are definitely not good enough to use your suggested method @elalish ... I would be really grateful for any hints.

antekj commented 4 years ago

So far, I was the closest to reaching my goal by changing this line const image = resizeImage( texture.image, needsPowerOfTwo, false, maxTextureSize ); to this const image = resizeImage( texture.image, needsPowerOfTwo, false, 1024 ); That improved the resolution of the background slightly but any number higher than 1024 either didn't change anything or made the background as blurry as initially.

suro7 commented 4 years ago

@cdata @elalish when can we expect this feature integrated into modelviewer? can you give us some hints how do implement/hack it before actual release? thanks

ozgurduygu commented 4 years ago

@elalish Thanks for working on it!

tadejskamp commented 4 years ago

@elalish - I pulled the branch highResSkybox and ran the build command. I now have the model-viewer.js file, but the the resolution of the 360 image (skybox) is still blured, -how do I enable the higher resolution?

suro7 commented 4 years ago

@elalish - I would appreciate if you send a modelviewer.js built file based on highResSkybox branch.

elalish commented 4 years ago

@tadejskamp I just pushed updated examples into that branch that should answer your questions. @suro7 if you'd like to test it early, feel free to pull the branch and follow our build instructions.

tadejskamp commented 4 years ago

@elalish - it works, Thnx!

antekj commented 4 years ago

Thank you so much @elalish . I am having difficulties trying to run the build command on my Windows 10 machine. Would you mind sharing the generated .js file, @tadejskamp ? I would appreciate it a lot. Thanks!

tadejskamp commented 4 years ago

@antekj - I had also problems trying to build it on Windows, then I succeded building it in Ubuntu-Linux VM.

dist-model.zip

antekj commented 4 years ago

@tadejskamp thank you so much! That's greatly appreciated. I can also confirm it's working beautifully :)