-
Notifications
You must be signed in to change notification settings - Fork 21
Pipeline and rendering with pipeline implementation #256
Conversation
@@ -0,0 +1,63 @@ | |||
/* Copyright (c) 2011-2014, EPFL/Blue Brain Project |
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.
review copyright
6db9e3f
to
e365b1e
Compare
* Executes the executable. Returns the futures that can be queried for data. | ||
* @param pipeline to be executed. | ||
*/ | ||
Futures execute( const ExecutablePtr& executable ) |
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.
const Ptr&?
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.
We've discussed this before. Yes, const Ptr& is the preferred way to pass smart pointers as function arguments because it saves the automic counter increment. Whether the pointer is finally stored by the callee is an orthogonal issue.
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, overall I should decrease the usage of Ptrs around. Lots of them makes things a bit fishy. Implementation are generally private so, it is possible to share them.
{ | ||
for( const auto& future: futures ) | ||
addFuture( future.getName(), future ); | ||
|
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.
Useless line? :)
{ | ||
const Boxf& worldBox = lodNode.getWorldBox(); | ||
const bool isInFrustum = _frustum.boxInFrustum( worldBox ); | ||
if( !isInFrustum ) |
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 don't know if it's better... But you can collapse these two line:
if( !_frustum.boxInFrustum( worldBox ))
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.
Actually I am not sure if it make things more clear...
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 does not add value but does not cloud information as well, so I rather keep it this way
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'm with chevtche in this one.
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 with my self but I would do !
+1 |
No description provided.