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

Update webgl_shaders_ocean.html #26580

Merged
merged 6 commits into from
Aug 16, 2023
Merged

Conversation

skyclouds2001
Copy link
Contributor

@skyclouds2001 skyclouds2001 commented Aug 14, 2023

Description

THREE.PMREMGenerator.prototype.fromScene method receive a scene param of Scene instance, see PMREMGenerator docs.
Even though, this incorrect param seem not to influence the final behavior of this example.

See https://threejs.org/examples/#webgl_shaders_ocean

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 14, 2023

Unlike WebGLRenderer.render(), PMREMGenerator.fromScene() expects an instance of Scene. That's because the internal _sceneToCubeUV() method sets the background property at the end of its implementation.

scene.background = background;

In the example, the sky object ends up with an undefined background property. Nothing breaks because of this but after all it is not ideal.

However, I don't think the PR is correct in its current form since the scene you are using is the same scene that holds the water and box. It's more correct to use a second scene, add the sky to it, execute PMREMGenerator.fromScene() and then add the sky back to its original scene. Or alternatively, you just use one scene but set the visible property of the water and box to false before fromScene() runs (and then restore the values).

@Mugen87 Mugen87 added this to the r156 milestone Aug 14, 2023
@skyclouds2001
Copy link
Contributor Author

skyclouds2001 commented Aug 15, 2023

Emmm, you are right. I'll update my pull request later.

@skyclouds2001 skyclouds2001 marked this pull request as ready for review August 16, 2023 11:44
@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 16, 2023

Slightly updated the PR.

@Mugen87 Mugen87 merged commit 8d9f8d5 into mrdoob:dev Aug 16, 2023
@skyclouds2001 skyclouds2001 deleted the skyclouds2001-patch-1 branch August 16, 2023 15:05
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.

2 participants