-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Enhancement: Tunnel Vision #450
Conversation
…gnette effect before applying to the renderTarget.
Adding @johnshaughnessy |
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.
Still reading thru rending code as it's all new to me. It looks like we add render passes, which I thought would make the FPS drop significantly, but in my initial (and not very thorough) tests it appears to run fine.
Left a few comments for things that can be improved in the meantime.
One other thought: We should allow the user to enable/disable this effect. A query string parameter might suffice, given that we don't have a settings menu or anything similar. I expect this is a polarizing feature, so we might find that we need to provide a user-friendly way of toggling it.
src/hub.html
Outdated
@@ -319,7 +318,8 @@ | |||
drawIncrementally: true; | |||
incrementalDrawMs: 600; | |||
hitOpacity: 0.3; | |||
missOpacity: 0.2;" | |||
missOpacity: 0.2; |
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.
Don't need these changes to teleport-controls attributes in this PR
THREE.EffectComposer = function(renderer, renderTarget) { | ||
this.renderer = renderer; | ||
this.delta = 0; | ||
window.addEventListener("vrdisplaypresentchange", this.resize.bind(this)); |
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.
Need to disable this effect when not in VR.
src/systems/tunnel-effect.js
Outdated
if (this.characterVelocity.distanceTo(STATIC) < CLAMP_VELOCITY) { | ||
// the character stops, so we use the aframe default render func | ||
this.scene.renderer.render = this.originalRenderFunc; | ||
this.isMoving = false; |
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.
The effect seems to "pop" at the end, when you stop moving. Can we fade away quickly and continuously? It only seems to be a problem when I stop moving. When I start moving, it fades in nicely.
Actually, both starting and stopping are 'pop'. The starting part looks good cuz you don't expect when it will show up and the radius becomes smaller and smaller after it starts. Currently, I don't come up with any good approach to fade out after the avatar stops since the vignette shader renders two circles together and no matter how big the radius is, you still can see the gap between two circles. |
|
src/systems/tunnel-effect.js
Outdated
Math.abs(r - TARGET_RADIUS) > CLAMP_RADIUS && | ||
Math.abs(softness - TARGET_SOFTNESS) > CLAMP_SOFTNESS | ||
) { | ||
if (this.deltaR !== 0 && this.deltaS !== 0) { |
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.
I think this line will only be true once, when this.deltaR
and this.deltaS
are undefined.
Is that desired?
src/systems/tunnel-effect.js
Outdated
}, | ||
|
||
_enterVR: function() { | ||
this.isVR = true; |
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.
Unfortunately if you press the "f" key without a headset plugged in, the "entervr" event still fires. You go into full screen, but you're not actually in VR. If you press F11 you can go into fullscreen mode without going into VR, but we may want to fix this anyway because we don't mean to use the vignette pass when we're not in vr.
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.
Address minor issues above then looks ready to push.
Any idea why the tunnel effect renders the scene very blurry without a headset plugged in? (In the current version of this system, will trick it into thinking it's in VR when it's not).
src/systems/tunnel-effect.js
Outdated
import "../utils/shaders/VignetteShader"; | ||
import qsTruthy from "../utils/qs_truthy"; | ||
|
||
const isDisabled = qsTruthy("disableTunnel"); |
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.
On by default, then disabled with a query parameter will get you the most feedback. Add a note in README.md under the ##Query Params header.
src/systems/tunnel-effect.js
Outdated
@@ -74,7 +74,7 @@ AFRAME.registerSystem("tunneleffect", { | |||
Math.abs(r - TARGET_RADIUS) > CLAMP_RADIUS && | |||
Math.abs(softness - TARGET_SOFTNESS) > CLAMP_SOFTNESS | |||
) { | |||
if (this.deltaR !== 0 && this.deltaS !== 0) { | |||
if (!this.deltaR && !this.deltaS) { | |||
this.deltaR = TARGET_RADIUS - r; | |||
this.deltaS = softness - TARGET_SOFTNESS; |
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.
I am still confused about why these are set once with some particular value for r
and softness
. Are deltaR
and deltaS
supposed to be constants? Do they need to be on this
? Given their names, I expected them to change whenever r
or softness
changed, but perhaps they're better understood to be fixed increments by which we will change r
or softness
every frame. In that case, should these constants be defined above with CLAMP_SPEED
, etc?
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.
the deltaR
is defined by the value of TARGET_RADIUS
and minRadius
. Since we define the minRadius
value in schema, I don't think it should be defined as a constant like CLAMP_SPEED
, but I init those delta values in init
in the latest commit.
8da161e
to
b52512d
Compare
|
utils/postprocessing
&utils/shaders
.systems/tunnel-effect.js
, an Aframe system that applies vignette postprocessing while the character is moving.