-
Notifications
You must be signed in to change notification settings - Fork 19
Deflect Qt: New OffscreenQuickView class (extracted from QmlStreamerImpl) #137
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
Conversation
@@ -0,0 +1,247 @@ | |||
/*********************************************************************/ | |||
/* Copyright (c) 2016, EPFL/Blue Brain Project */ | |||
/* Raphael Dumusc <raphael.dumusc@epfl.ch> */ |
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 did 80% of this file, for shame :)
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 did it for my glory, for which I am grateful :-)
deflect/qt/OffscreenQuickView.cpp
Outdated
connect( _qmlComponent.get(), &QQmlComponent::statusChanged, | ||
this, &OffscreenQuickView::_setupRootItem ); | ||
|
||
// Now hook up the signals. For simplicy we don't differentiate between |
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.
typo 'simplicy'
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.
done
deflect/qt/OffscreenQuickView.cpp
Outdated
|
||
QImage OffscreenQuickView::getImage() const | ||
{ | ||
return _quickRenderer->fbo()->toImage(); |
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.
no check if fbo() is valid?
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 has no reason to be invalid at this point and I don't know what to do if it is not... throw?return an empty image?
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.
After constructor or if render target is not FBO this will be nullptr, so at least empty image to not crash. Maybe also add more documentation in the header in which cases this is actually valid.
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're right, and I realize it is also not very safe that this can only be called from a Qt::DirectConnection (while the FBO is bound). A better way would be to get the image internally just afterRender() and always keep a copy, so that class users don't risk making any mistake.
deflect/qt/OffscreenQuickView.cpp
Outdated
connect( this, &QQuickWindow::heightChanged, | ||
_rootItem.get(), &QQuickItem::setHeight ); | ||
|
||
// also bind view to root object, to allow progamatic resize of the rootItem |
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.
typo 'progamatic'
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.
done
deflect/qt/QmlStreamerImpl.cpp
Outdated
QmlStreamer::Impl::Impl( const QString& qmlFile, const std::string& streamHost, | ||
const std::string& streamId ) | ||
: _renderControl( new QQuickRenderControl ) | ||
// Create a QQuickWindow that is associated with out render control. Note |
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.
that comment does not apply here anymore
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.
done
No description provided.