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

WebGLRenderer: Add targetScene parameter to compile() methods. #26966

Merged
merged 1 commit into from
Oct 15, 2023

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Oct 14, 2023

Related issue: #19752

Description

This PR implements the feedback from #19752 (comment) and introduces the targetScene parameter, which was part of the original version of #19752.

@toji I've managed to refactor the code such that compileAsync() reuses the implementation from compile() which makes the code more manageable. Do you think your use case will work with this version of compileAsync()?

BTW: Existing usage of compile() should not be affected by this change. targetScene is optional.

@@ -1036,15 +1004,15 @@ class WebGLRenderer {

const material2 = material[ i ];

prepareMaterial( material2, scene, object );
compiling.add( material2 );
prepareMaterial( material2, targetScene, object );
Copy link
Collaborator Author

@Mugen87 Mugen87 Oct 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a subtle but important change. We have to use targetScene here since otherwise we miss environment definitions (and then end up with recompilations during the first render).

@Mugen87 Mugen87 added this to the r158 milestone Oct 14, 2023
@github-actions
Copy link

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
652.3 kB (161.6 kB) 652.2 kB (161.6 kB) -133 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
445 kB (107.6 kB) 444.9 kB (107.6 kB) -133 B

@toji
Copy link
Contributor

toji commented Oct 14, 2023

This would work for my use case, yes! (And hopefully many others as well!)

I really like the change to have compileAsync() call into the standard compile(). I wasn't sure how much leeway there was to change it when I put up my previous PR, which is why it duplicated the majority of the logic, but this pattern is much cleaner and more maintainable. +💯

@Mugen87 Mugen87 merged commit 307e2e6 into mrdoob:dev Oct 15, 2023
19 checks passed
@Mugen87
Copy link
Collaborator Author

Mugen87 commented Oct 15, 2023

Thanks for the review! And sorry for deleting targetScene in my first refactoring 😇 . TBH, I did not fully understand its added value but I can see now how it makes the compile() method more versatile!

@toji
Copy link
Contributor

toji commented Oct 15, 2023

Thank you! I'm glad that we ended up going through this review separately in the end, because I think having this call through to 'compile()' makes the implementation better.

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