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

Scene: Add background blur with PMREM. #24752

Merged
merged 1 commit into from
Oct 12, 2022
Merged

Scene: Add background blur with PMREM. #24752

merged 1 commit into from
Oct 12, 2022

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Oct 6, 2022

Related issue: #23712

Description

This is the alternative implementation of #23712 but via PMREMs.

@WestLangley
Copy link
Collaborator

I support your PR in principle, but not how it has been shoehorned into the existing shaders.

I have been trying to clean up the shaders, and this is not helping.

I am hopeful that if this is merged, we can work together on some shader refactoring.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 6, 2022

Definitely. IMO when adding new features, there is no need to make the implementation perfect right from the beginning. As long as the basic approach is right, we can use refactoring to fine-tune things at a later point.

@mrdoob mrdoob added this to the r146 milestone Oct 7, 2022
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 12, 2022

Let's merge so we can easier move on with the suggested refactoring.

@Mugen87 Mugen87 merged commit c22c1fc into mrdoob:dev Oct 12, 2022
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 12, 2022

Updated builds: 4e93f9b

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 12, 2022

About the refactoring: I guess the goal is that the background material does not depend on ShaderLib.cube anymore. Right now ShaderLib.background is only used for simple textured backgrounds. Are you okay with extending it so it also handles env maps?

@WestLangley
Copy link
Collaborator

ShaderLib.background is only used for simple textured backgrounds. Are you okay with extending it so it also handles env maps?

I think that would be OK. I am assuming it would be used exclusively by scene.background.

so it also handles env maps...

Nit: I would prefer to say it this way: "so it also handles texture cubes and equirectangular textures".

@WestLangley WestLangley mentioned this pull request Oct 16, 2022
@joshuaellis joshuaellis mentioned this pull request Oct 31, 2022
19 tasks
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.

3 participants