Hi Robert,
I agree with the method naming remarks. A setter should have a set prefix.
It is a little bit strange to use an image cache, don't use a
SharedStateManager and use all the same a PixelBufferObject but it is
possible so changing the hasBeenRead boolean to a "user counter" seems
appropriate.
The implementation will be a little bit different from boolean one. This
counter should be initialized to the number of textures referencing the
image.
The counter cannot replace the hasBeenRead member of BufferEntry. They
are created lazily
during the texture application or compilation. The number of "users" is
unknown at this time.
The counter cannot be placed inside BufferObject because the
IncrementalCompileOperation is unable to know the number of textures
referencing the processed image while it is setting up the PBO.
The counter can be placed inside BufferData. The texture can keep it up
(inside setImage, the constructor and the destructor).
The counter should be made contextual. Texture::setImage should
increment the counter
of all the contexts and Texture::apply should decrement only the counter
of current
context.
The counter looks like a contextual version of the texture ref_ptr on
the image.
Comments?
Lionel.
On 30/05/2013 11:55, Robert Osfield wrote:
Hi Lionel,
I have just done another review of your changes. I am now pretty
comfortable with the general approach but not fully with the naming
and implementation.
Having a bool per BufferDataEntry works fine for cases where only one
OpenGL object will read from buffer, while this is the usual case,
might there be cases where one BufferData object might be shared -
i.e. two Texture's sharing the same osg::Image. While this isn't
optimal, it still can happen in scene graphs if a loader doesn't do
it's job efficiently are sharing state, I mention this as the
OpenFlight plugin is a real culprit for this and needs optimizing to
avoid duplicate state. Given the possibility of sharing BuferData I
do wonder if a uint might be more appropriate Such a change will
require us to increment the number of reads required each time we
associated a BufferData object with a GL object which could get kinda
sticky.
The naming of hasAllBufferDataBeenRead() is fine, but I
hasReadBufferData(const osg::BuferData*) doesn't quite work for me as
it it's a bit ambiguous - it sounds like a question, but in fact it's
a statement of fact and really should read clearly as such.
setBufferDataHasBeenRead(const osg::BuferData*) might be more clear.
However, the naming would all change if we decide to go for handling
that possibility of sharing a BufferData object between multiple
arrays or textures.
Thoughts?
Robert.
_______________________________________________
osg-submissions mailing list
[email protected]
http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
_______________________________________________
osg-submissions mailing list
[email protected]
http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
