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

Textures appear black and reflections are off on scenes with reflection probes #5570

Closed
matthewbcool opened this issue Jul 6, 2022 · 13 comments
Assignees
Labels
bug needs triage For bugs that have not yet been assigned a fix priority P1 Address as quickly as possible

Comments

@matthewbcool
Copy link
Contributor

Description
Environment map reflections appear incorrectly positioned and textures are black in high quality mode

To Reproduce
Steps to reproduce the behavior:

  1. Go to https://hubs.mozilla.com/scenes/gHuNHUI/cliffsideretreat
  2. Spawn in a camera, 3d object with PBR materials and observe

Expected behavior
Materials reflections/materials should be more consistently lit with env maps IBL.

Screenshots
a40b7e6b-3303-405f-a191-717dcf9b6b4f
Screen Shot 2022-07-06 at 4 06 11 PM

90025f635997a4dc53a9a98ea22c7827.mp4

Hardware

  • Device: [MacBook M1]
  • Browser: [Chrome, Firefox]

Additional context
also tested on PC, windows Firefox

@matthewbcool matthewbcool added bug needs triage For bugs that have not yet been assigned a fix priority labels Jul 6, 2022
@takahirox takahirox self-assigned this Jul 6, 2022
@takahirox
Copy link
Contributor

Thanks for the problem report. Probably it was broken by the recent Three.js upgrade #5488. Let me investigate...

@matthewbcool matthewbcool changed the title Textures appear black and reflections are off on scenes with env. maps Textures appear black and reflections are off on scenes with reflection probes Jul 8, 2022
@matthewbcool
Copy link
Contributor Author

Scenes with only an env map and no reflection probes don't seem to have this problem: https://hubs.mozilla.com/scenes/HsSRCtj/merry-go

@takahirox
Copy link
Contributor

Yes, light probe is suspicious. And I noticed that commenting this line out makes the objects non-black https://github.com/mozilla/hubs/blob/d815053b1938068d54fda517c0f9af0760423b09/src/gltf-component-mappings.js#L613

@takahirox
Copy link
Contributor

Reflection Probe texture (envMap) passed to WebGL is very suspicious...

Left r131, right r141.

image

@takahirox
Copy link
Contributor

/cc @netpro2k

@emclaren
Copy link
Contributor

is this possibly related to #5571

@emclaren emclaren added the P1 Address as quickly as possible label Jul 12, 2022
@takahirox
Copy link
Contributor

is this possibly related to #5571

Maybe not

@takahirox
Copy link
Contributor

takahirox commented Jul 13, 2022

Ah, probably I figured out the root issue finally. The reflection proble patch has conflicts in the logic (algorithm) with Three.js r141. This is the reason why I don't like applying patches to Three.js, it's very hard to figure out the root issue if having no conflict in the code but having conflicts in logic (algorithm)... (I have spent about a week for this problem.)

I'll share the details soon.

@takahirox
Copy link
Contributor

takahirox commented Jul 14, 2022

If I'm right the root issue seems to be switching environment maps after setting up WebGL programs in the renderer.

The renderer and other webgl modules generate shader code, get program cache, decide whether changing program, and so on, from material (and other objects) properties.

Switching envMap after setting up them can cause mismatch.

More proper implementation may be setting up envMapA/B first and include them for decision of shader code generation, program cache, and so on.

@netpro2k
Copy link
Contributor

Looks like the root of this regression was this change in Three 138 mrdoob/three.js#23322 ... Previously PMREM textures had a fixed size. They are now variable so we need to further extend our multi-environment map support to account for this.

I think we can just have it be a know limitation for now that all reflection probes and environment maps have to be the same resolution. We can enforce this in the Blender addon.

Ultimately we probably can support this when we do another pass at reflection probes.

@takahirox
Copy link
Contributor

A workaround PR for Hubs Blender add-on has been opened Hubs-Foundation/hubs-blender-exporter#88

@takahirox
Copy link
Contributor

Opened an issue for the root issue of Reflection Probe implementation Hubs-Foundation/three.js#66

We may close this issue after Hubs-Foundation/hubs-blender-exporter#88 lands.

@takahirox
Copy link
Contributor

Closing this issue because Hubs-Foundation/hubs-blender-exporter#88 has been merged.

Please re-export your scenes if you encounter the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs triage For bugs that have not yet been assigned a fix priority P1 Address as quickly as possible
Projects
None yet
Development

No branches or pull requests

4 participants