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

Move EffectComposer "Pass" into its own file #14795

Closed
wants to merge 4 commits into from

Conversation

gkjohnson
Copy link
Collaborator

What Changed

  • Moves THREE.Pass definition from postprocessing/EffectComposer.js to postprocessing/Pass.js
  • Add warning to EffectComposer if THREE.Pass isn't defined to notify users of the change
  • Update all examples that use the EffectComposer to include the new file

Why

I'm looking into making a script to convert all the example scripts into es6 module files and this file was proving troublesome because it creates cyclic dependencies:

EffectComposer (EffectComposer.js) depends on ShaderPass which depends on Pass (EffectComposer.js).

So EffectComposer.js imports ShaderPass before it has defined Pass, yet, which causes ShaderPass to throw an undefined variable error. Moving Pass out the EffectComposer file removes the cyclic dependency.

@WestLangley
Copy link
Collaborator

Is there a way to inline the code if it is not defined so existing fiddles will continue to work?

Can you swap the order?

<script src="js/postprocessing/EffectComposer.js"></script>

<script src="js/postprocessing/Pass.js"></script>
<script src="js/postprocessing/ShaderPass.js"></script>
<script src="js/postprocessing/MaskPass.js"></script>

@gkjohnson
Copy link
Collaborator Author

Is there a way to inline the code if it is not defined so existing fiddles will continue to work?

I could do that by copy / pasting the code from Pass into EffectComposer and then printing a warning if it's been inlined that it should actually be included separately. I suppose another approach would be to fetch and eval the Pass script if it doesn't exist but that feels dirty. It would keep the definitions in sync, though.

Can you swap the order?

You can swap the order and everything will work but the warning will print if Pass is included after EffectComposer. I tried putting the warning inside of the EffectComposer constructor but other code would fail from the missing Pass definition before the EffectComposer was created in the examples so the warning would never get printed, making kinda useless.

Is there a better way to handle the printed warning?

@gkjohnson
Copy link
Collaborator Author

Is there a better way to handle the printed warning?

I suppose I could wrap the log in a requestAnimationFrame so the the other scripts have had a chance to load before checking if Pass exists. That way it won't matter what order the scripts are included in.

@gkjohnson
Copy link
Collaborator Author

/ping @mrdoob @WestLangley

Any further thoughts on what I should do here? Should I copy the Pass source into EffectComposer and wrap the warning in requestAnimationFrame?

Thanks!

@WestLangley
Copy link
Collaborator

Sorry, I am not sure of the preferred way to handle this...

@Itee
Copy link
Contributor

Itee commented Sep 11, 2018

+1 For this PR ! It will allow to remove a patch from three-full. In my view Pass should be in his own file, clearly.

@gkjohnson gkjohnson closed this Jun 10, 2019
@gkjohnson gkjohnson deleted the separate-composer-pass branch June 10, 2019 16:49
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