Livre histogram support #301
Conversation
bilgili
force-pushed
the
histogram
branch
9 times, most recently
from
7b0ba4b
to
5191d06
Compare
|
Why did you add a Doxyfile? |
|
|
||
| namespace | ||
| { | ||
| const size_t binCount = 256; |
There was a problem hiding this comment.
Aren't we using uppercase names for constants?
Declare it as a static const member of Histogram instead of a global variable.
There was a problem hiding this comment.
I was using the variable, but yes I can put into the class.
There was a problem hiding this comment.
Since you already have the static member function in the class I would replace the function by a const static member and remove this one.
|
You haven't answered why you needed to add a Doxyfile. |
| setId( histogram.getId( )); | ||
| setArea( histogram.getArea( )); | ||
| _minIndex = -1u; | ||
| _maxIndex = -1u; |
There was a problem hiding this comment.
Why don't you assign the values from the right hand side histogram?
| bool isCenterInViewport( const Frustum& frustum, | ||
| const Boxf& worldBox, | ||
| const Vector3f& minNdc = Vector3f( -1.0f ), | ||
| const Vector3f& maxNdc = Vector3f( 1.0f )) const |
There was a problem hiding this comment.
Nope, I am not seperating impl classes into privates etc.
| const size_t binCount = _histogram->getBinsSize(); | ||
| const float minVal = std::numeric_limits< SRC_TYPE >::min(); | ||
| const float maxVal = std::numeric_limits< SRC_TYPE >::max(); | ||
| const float range = maxVal - minVal; |
There was a problem hiding this comment.
For holding a 32 bit integer range these variables should be double.
| @@ -118,7 +149,7 @@ struct Channel::Impl | |||
| public: | |||
| explicit Impl( Channel* channel ) | |||
| : _channel( channel ) | |||
| , _frameInfo( Frustum( Matrix4f(), Matrix4f()), INVALID_FRAME ) | |||
| , _frameInfo( Frustum( Matrix4f(), Matrix4f()), INVALID_TIMESTEP, INVALID_TIMESTEP ) | |||
There was a problem hiding this comment.
Minor comment: you could use a default constructor in FrameInfo for this.
|
Just some very minor comments left. |
No description provided.