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

Screenshot WIP for feedback #179

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

msfeldstein
Copy link
Contributor

This still has work that needs to be done but i wanted to get some feedback

Copy link
Contributor Author

@msfeldstein msfeldstein left a 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"
Copy link
Contributor Author

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'
Copy link
Contributor Author

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 );
Copy link
Contributor Author

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) {
Copy link
Contributor Author

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.

@@ -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'],
Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

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')
Copy link
Contributor Author

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)
Copy link
Contributor Author

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.

Copy link
Member

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();
Copy link
Contributor Author

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.

@dmarcos dmarcos mentioned this pull request Jan 20, 2017
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