Hi Sukender, I've just done a review of your changes and I presently don't feel that it's appropriate to change the texture unit numbers of loaded models. As separate post process it would be fine, but if a modeller has created a scene that uses textures on particular texture units and shaders that are associated with these then problems will ensure.
I believe it would be far safer to have an optional post process of these scene graphs packing the StateSets so that the texture units are remapped in a user controallable way. On the topic of the RAII structure your created, this seems rather over complicated for what could be achieve with a simple std::auto_ptr<>. The only twist in the method that you modified is that whether the DAE object is locally created or not. For this you could simply create the auto_ptr<> in the scope of the function and then assign the DAE pointer to the auto_ptr<> in the local scope that creates the the DAE, but not when you get it from the Options structure. Aleternatively you could assign a NULL to the auto_ptr<> when the DAE object is not ownload locally. Robert. On Tue, Mar 8, 2011 at 3:29 PM, Sukender <[email protected]> wrote: > Hi Robert and Collada submitters/users, > > Warning: This submission implies my previous one has been integrated (See > "[osg-submissions] DAE Reading fix for path containing '#'"). > Base rev: trunk 12208 > > Commit message: > - Made Collada reader not create StateSets with an empty texture unit BEFORE > a valid one (Ex: Texture unit 0 empty, texture unit 1 containing diffuse). > Actually previous implementation is valid and may work for many usages, but > unfortunately some writers only support texture unit "0" and thus will skip > textures of simple models made with only a diffuse texture. > - I also spotted ReaderWriterDAE::readNode() was missing a "if (bOwnDAE) > delete pDAE;" near line 71 (before "return > ReadResult::ERROR_IN_READING_FILE;") and thus may lead to a memory leak. In > order to avoid only handling this case, I added a RAII structure to > deallocate "pDAE". I know it's a bit a burden having such code, but as > ColladaDom is C++ (contrary to 3DS which is C) and may throw exceptions, I > think this is cleaner. > > Collada submitters/users, please test files containing AMBIENT_OCCLUSION_UNIT > if possible, since I don't have any model to test. Changes are quite > straightforward but this should be tested anyway. > > Cheers, > > Sukender > PVLE - Lightweight cross-platform game engine - http://pvle.sourceforge.net/ > > _______________________________________________ > 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
