H Sukender, Do you want me to dive in and do review of this submission or might there be further updates to your version of dae plugin since posted these latest changes on the 18th of April?
Cheers, Robert. On Mon, Apr 18, 2011 at 12:30 PM, Sukender <[email protected]> wrote: > Hi Robert, > > I wrapped my change about non-empty texture units in an off-by-default > option, so that it doesn't break any existing code. But AFAIK, other readers > (except OSGx) generate contiguous texture units, so I guess this option to be > useful for users. > > Other changes in this sumbission include the added ability to tessellate > polygons in Collada reader, with appropriate options. > - No tessellation > - Tessellate as triangle fan (previous behaviour, kept as default for > backward-compatibility) > - Full tessellation > > I also put auto_ptr<> for RAII of DAE structure (as discussed), and moved > reader options in a structure, as for the writer. > > Code also make use of osgDB::PluginImageWriter I submitted, so please merge > this submission after the PluginImageWriter one. > > I hope this will be not too heavy to merge (it seems you're very busy and/or > in vacation these days! ;) ) > Cheers, > > Sukender > PVLE - Lightweight cross-platform game engine - http://pvle.sourceforge.net/ > > ----- "Robert Osfield" <[email protected]> a écrit : > >> Hi Sukender, >> >> I can't really answer these questions on what should be the default >> as >> I don't have experience with using Collada supporting tools in a >> production environment. What I do know is that changing how things >> work is likely to cause current end users to have to accomodate this >> change, as textures will now be assigned to different textures units. >> >> My inclindation is to have the dae plugin make sure that for each dae >> file it uses one convention for each texture type, and for this >> convention to be customizable during load, or as a post process. To >> do texture unit resassignment as a post process the loaded scene >> graph >> will need to contain details on what the units that have been >> assigned >> map too. For the later perhaps osg::Node description field could be >> used, or to have a custom UserData that provide this information in a >> way that application developers can load the scene graph and then >> account for the mapping that it contains. An osgUtil visitor would >> also be required to enable convinient remapping. >> >> What would be best as defaults I wouldn't like to say until I get >> more >> some wider from other COLLADA users in the community. A discussion >> on >> osg-users would be appropriate. >> >> Robert. >> >> >> On Thu, Mar 10, 2011 at 12:48 PM, Sukender <[email protected]> wrote: >> > 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 >> > >> _______________________________________________ >> 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
