-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
Remove sphericalEnvMap #4747
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
Remove sphericalEnvMap #4747
Conversation
|
One test is failing/timing out on Firefox: adding AR state. The same test is failing on master, however, so it’s unrelated to this PR. |
|
So I did a bit of exploration and it looks like this is used quite a lot in the community: https://github.com/search?q=sphericalEnvMap&type=code. In this case, perhaps the best way would be to provide a translation layer so these keep working? I don't think I described it clearly enough initially: |
ac48c2b to
99acd60
Compare
|
Just rebased this upon the latest master, so that specs pass. Still interested in feedback about the actual contents of the PR!
|
|
Thanks and sorry for the delay. If |
|
Three.js's own migration notes include this notice: "SphericalReflectionMapping is no longer supported. Consider using a Matcap texture with MeshMatcapMaterial instead." For future reference, this is the PR that removed it + reasons: mrdoob/three.js#19517 I think the
I'm not 100% sure though - let me know if you think a different approach would be better, happy to fix it! |
|
@jure I know this is a bit old. Maybe we're in a better position now to make a decision. Do you think we should finally remove spherical maps or offer an alternative? Are cube maps the way to go and a suitable replacement? Should we switch to |
|
closing due to inactivity |
Description:
The
THREE.SphericalReflectionMappingmapping thatsphericalEnvMaprelies on has been removed in Three.js (mrdoob/three.js#19517) and the library was released without it in r118, so reflections using this feature no longer work in the latest release of A-Frame.The cited reasons are (and I'm somewhat paraphrasing) that it almost never does what the user expects it to do or is otherwise ill-suited. And that using cube maps or something like https://github.com/colinfizgig/aframe_Components/blob/master/components/camera-cube-env.js, i.e. camera-generated
envMaps is a better approach for reflections.There are three options as I see it:
sphericalEnvMapTHREE.SphericalReflectionMappinginsuper-threeThe PR implements option 1, as the upstream reasons for removal make sense to me, but happy to discuss further!