aframe icon indicating copy to clipboard operation
aframe copied to clipboard

Remove sphericalEnvMap

Open jure opened this issue 4 years ago • 6 comments

Description:

The THREE.SphericalReflectionMapping mapping that sphericalEnvMap relies on has been removed in Three.js (https://github.com/mrdoob/three.js/pull/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:

  1. Removing sphericalEnvMap
  2. Reintroducing THREE.SphericalReflectionMapping in super-three
  3. Some sort of translation layer into cube maps

The PR implements option 1, as the upstream reasons for removal make sense to me, but happy to discuss further!

jure avatar Dec 11 '20 22:12 jure

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.

jure avatar Dec 12 '20 09:12 jure

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: sphericalEnvMap is currently broken in 1.1.0.

jure avatar Dec 14 '20 16:12 jure

Just rebased this upon the latest master, so that specs pass. Still interested in feedback about the actual contents of the PR!

  1. Should we remove sphericalEnvMap as this PR does?
  2. Should we provide a fallback for it, even though Three.js does not support it anymore?

jure avatar Dec 21 '20 22:12 jure

Thanks and sorry for the delay. If sphericalEnvMap is gone. Should we have an alternative or at least recommend one in the docs / faq?

dmarcos avatar Jan 05 '21 14:01 dmarcos

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: https://github.com/mrdoob/three.js/pull/19517

I think the MeshMatcapMaterial migration suggestion above is for when spherical reflection mapping is "correctly" used - I expect a fair number of projects use it incorrectly though and would be better served by envMaps, so I've updated the docs to suggest envMap as the recommended alternative, as that's probably what people want, anyway:

Notice: The previously available sphericalEnvMap property was removed because support no longer exists in Three.js (PR): we recommend users switch to envMap for environment reflections.

I'm not 100% sure though - let me know if you think a different approach would be better, happy to fix it!

jure avatar Jan 05 '21 15:01 jure

@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 MeshMatcapMaterial as recommended in the migration guide?

dmarcos avatar Dec 07 '21 02:12 dmarcos