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

BufferObject Configuration serialization #234

Closed
wants to merge 2 commits into from

Conversation

mp3butcher
Copy link
Contributor

@mp3butcher mp3butcher commented Mar 28, 2017

-add a flag in Outpustream in order to know whether or not BufferObject associated with BufferData should be serialized
It can be usefull in order to save custom BufferObject configuration.
-Changes involved in BufferData serializers

Details
in bd serilializer (use a trick not to add BufferData to BufferObject
///don't add BufferData to BufferObject (let BufferObject Serializer::readBufferData do it)
static bool readBufferObject( osgDB::InputStream& is, osg::BufferData& bd );

in bo serializer (do the job of adding BufferData to BufferObject)
/// add BufferData to BufferObject (finish job began in BufferData serilizer::readBufferObject)
static bool readBufferData( osgDB::InputStream& is, osg::BufferObject& node )

I doubt adding a bool flag in Outpustream dedicated to BufferObject serializer is the cleaner way to control bo serilization but you surely the my point.

With It I'm able to serialize complex patterns such as transformfeedback or indirectdraw so i though it was cool enough to raise your interest for this feature

…t associated with BufferData should be serialized

It can be usefull in order to save custom BufferObject configuration.
-Changes involved in BufferData serializers
@openscenegraph
Copy link
Owner

There are number of things about this submissions that make my hair stand on end. It strikes me as really fighting the intended design of the osg::BufferObject/BufferData relationship, you either have an odd usage case or a misunderstanding of the basic design and implementation. Either way I don't think pushing PR's that I'll reject is the way forward, if there is usage case you don't currently feel is supported please raise it on the osg-users mailing list/forum for general discussion.

The only changes that I'm happy with are the ones adding BufferData to the PrimitiveSet classes, this seem reasonable and uncontroversial. Could you separate out these changes into a separate PR so I can go ahead and merge these. Thanks.

fix declaration of the     bool _writeBufferObjectConfiguration; in osgDB:OutputStream
@mp3butcher
Copy link
Contributor Author

mp3butcher commented Mar 28, 2017

if there is usage case you don't currently feel is supported please raise it on the osg-users mailing list/forum for general discussion.

I already talk about it on the forum but you obviously don't see my point.
I don't know in what this contribution is akward: if you look at it there are only tricks that make the same thing we'll do by hand :
by code bd->setBufferData(bo) ///hide details of setting index, bo->addBufferData(ba) and set bo ownership
in the serializers
BufferData serialiser (set the index and bo ownership)
BufferObject serialiser (add the bufferdata to the bo)
(bd appears as bo property only to finish the job of addingBufferData to the bo it has no ownership of these)

@openscenegraph
Copy link
Owner

In your PR your BufferData_serializer_BufferData exists to workaround the design/implementation detail of BufferData/BufferObject, it's a hack, it's a clear indication that core design doesn't support what you are wanting to do. Fixing a usage limitation in the core implementation by hacks elsewhere is not a good approach and will almost certainly come back to bite us. The same applies to the other changes that I haven't merged, as things stand these changes aren't appropriate to merge.

@mp3butcher mp3butcher deleted the BOserialize branch January 4, 2018 03:32
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.

None yet

2 participants