Hi Robert, About the texture units, please note that this is something coming from the reader, not the Collada file. Actually, if you have, say, "texture unit 0 = diffuse" in your file, then the current importer will create an empty texture unit at pos 0, and a diffuse texture unit at pos 1. I don't belive it's a good behaviour, IMHO (Other readers don't have such "fixed units" behaviour, AFAIK).
About the fact these texture units may be hardcoded in shaders, I propose the behaviour to be optional: 1. Put textures in fixed units, depending on their usage (current behaviour) 2. Put textures in units as they come (my submission) With my code, this would be easy to do. Do you agree? Which one should be the default? Anyway, a feature (somewhere in osgUtil / osgUtil::Optimizer?) which moves units to "fill holes" would be great... About the RAII structure: "silly me!". Yes of course you're right. This was a direct transcription of the BOOST_SCOPE_EXIT() I use in my personal code. I'll change that after we have discussed the other point. Thank you for your feedback. Cheers, Sukender PVLE - Lightweight cross-platform game engine - http://pvle.sourceforge.net/ ----- "Robert Osfield" <[email protected]> a écrit : > 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 _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
