-
Notifications
You must be signed in to change notification settings - Fork 200
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
Screenshot WIP for feedback #179
base: master
Are you sure you want to change the base?
Conversation
…button in the model
…g and disabling each component
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 work to do before committing but i wanted to get feedback on my direction.
ui></a-entity> | ||
<a-entity id="right-hand" | ||
brush | ||
if-no-vr-headset="visible: false" | ||
paint-controls="hand: right" | ||
screenshot-camera | ||
trigger-mode="brush" |
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.
Perhaps this should be called action-mode since it might support more than just the trigger button, IE using the grips for scaling instead of undo
@@ -65,7 +68,14 @@ AFRAME.registerComponent('brush', { | |||
} | |||
}); | |||
}, | |||
|
|||
triggerModeChanged: function() { | |||
this.enabled = this.data.enabled && this.el.getAttribute('trigger-mode') == 'brush' |
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.
Is the proper way to do this to have another component that manages the trigger-mode attribute to data.enabled binding? To make this more reusable it would be nice if this didn't have to worry about the external trigger-mode attribute
var _gl = renderer.context | ||
var framebuffer = renderer.properties.get(this.renderTarget).__webglFramebuffer | ||
_gl.bindFramebuffer( _gl.FRAMEBUFFER, framebuffer ); | ||
_gl.readPixels( 0, 0, width, height, _gl.RGBA,_gl.UNSIGNED_BYTE, this.pixels ); |
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 couldn't get WebGLRenderer.readRenderTargetPixels to work here, unsure why as this looks like the same code. Is there a good way to debug Threejs while working on a-painter?
}, 'image/png'); | ||
}, | ||
|
||
flipPixelsVertically: function (pixels, width, height) { |
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.
This flipping seems to cause frame skipping. It actually kind of works because the white flash feels like taking a flash photograph, but it should probably be moved into a web worker or something.
src/components/ui.js
Outdated
@@ -1,7 +1,7 @@ | |||
/* globals AFRAME THREE */ | |||
AFRAME.registerComponent('ui', { | |||
schema: { brightness: { default: 1.0, max: 1.0, min: 0.0 } }, | |||
dependencies: ['ui-raycaster'], | |||
dependencies: ['ui-raycaster', 'screenshot-camera', 'brush'], |
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.
Is there a reason brush wasn't a dependency before?
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.
'brush' is a dependency of paint-controls
. Any reason do you need it here as a dependency? I would do a new component screenshot-controls
that it's enabled when clicking on the screenshot
button. The ui
component manages what controls are enable/disable. For instance: You click on screenshot
button then you disable paint-controls
and enable screenshot-controls
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 i misunderstood dependencies. Is ui-raycaster
needed as a dependency because we modify it in the init() method?
And are you thinking screenshot-controls
would be different than the screenshot-camera
component i added, or I should just rename it?
toggleCaptureCamera: function() { | ||
var enableCamera = this.handEl.getAttribute('trigger-mode') != 'camera' | ||
this.handEl.setAttribute('trigger-mode', enableCamera ? 'camera' : 'brush') | ||
this.handEl.emit('trigger-mode-changed') |
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.
Is there a built-in way to listen for any attribute changes, or do i need to emit custom events each time i change?
src/index.js
Outdated
requestAnimationFrame(f) | ||
AFRAME.TWEEN.update() | ||
} | ||
requestAnimationFrame(f) |
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.
This is just to fix the chrome animation frame timing bug, will remove before committing.
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.
You could develop on Firefox Nightly now and you don't need it and get better latency ;)
@@ -156,6 +156,7 @@ AFRAME.registerComponent('ui', { | |||
} | |||
case name === 'clear': { | |||
if (!this.pressedObjects[name]) { | |||
this.toggleCaptureCamera(); |
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 get another button in the menu for this. If people like the change maybe @feiss can hook up a camera button.
This still has work that needs to be done but i wanted to get some feedback