-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add Haze Pass #41
base: master
Are you sure you want to change the base?
Add Haze Pass #41
Conversation
app.js
Outdated
@@ -85,6 +87,8 @@ class App { | |||
this.infoDetailsClose = document.getElementById("info-details-close"); | |||
this.infoDetailsDisplayed = false; | |||
|
|||
this.urlParams = new URLSearchParams(window.location.search); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind moving the new url params into main.js so they're handled the same as the current url params (the pattern is that the url params are parsed in main.js and loaded into an "options" object which is passed to the App construtor).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much cleaner, done.
@@ -693,22 +697,67 @@ class App { | |||
|
|||
this.scene.add(this.camera); | |||
|
|||
this.composer = new THREE.EffectComposer(this.renderer); | |||
// Set up render target for holding depth information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great to make all the new composers/passes conditional on a url param, so if it's not enabled, the behavior/rendering is unchanged from before. This will make it easier to deploy this new feature while we experiment with and tweak it, without changing the current user experience. (At the moment it looks to me like there is a bit of a haze effect, or maybe it's just the antialising, I'm not sure, even if the new haze & depth params aren't present in the url. Also, eventually we might want different haze settings for birds-eye vs street-level views. In any case, if we can make it so that none of this is enabled, and the user sees the same result as before, unless they add special url params, then we can go ahead and deploy it and start experimenting with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good note, I'll work on that. I think the current "haze" when the haze feature is disabled actually comes from the softening of the edges with the antialising from a previous merge, but I'll double check that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a city to look more alive, we should include a slight environmental haze to indicate depth.
I added two passes - depth pass and haze pass.
These two passes rely on accessing the depth buffer, which needs to be rendered to in a separate composer pipeline.
Depth Change
The depth pass displays the depth buffer which is useful for debugging some rendering effects. This can be enabled by appending
&debug=true&depth=true
to the URL parameters.Haze Change
The haze pass adds a haze to the final image, which depends on the depth (objects further away are hazier, closer are clearer). This can be enabled by appending
&haze=true
to the URL parameters.Before (No Haze)
After (Haze)