No reply so have gone ahead and merged without tagging a specific name.
On 17 June 2014 10:22, Robert Osfield <[email protected]> wrote: > Hi Sam, > > You fix looks sensible. I am ready to merge then change but will wait > for clarification of the name you'd like me to assign for the > submission - when I merged a submission from users I used the commit > message of the form : "From Joe Blogs : bug fix that makes the OSG > 1,000,000 times faster". > > So could you let me know your full name or a psuedo name that you plan > to use consistently. > > Cheers, > Robert. > > > On 17 June 2014 07:48, Samuel M <[email protected]> wrote: >> >> Hello, >> >> while the MergeGeometryVistor condition was patched, it still contains a >> bug, effectively using the entire list of geometries to merge, which is >> wrong. >> >> Attached Optimizer.cpp patches this. This is a modified revision 14262. >> >> >> Regards, >> Sam >> >> P.S: Sorry if this mail is sent twice. I had issues with the subscription. >> >> >>> Date: Mon, 16 Jun 2014 17:19:20 +0100 >>> From: [email protected] >>> To: [email protected] >>> Subject: Re: [osg-users] MergeGeometryVisitor behaviour, is this a bug? >>> >>> Hi Sam, >>> >>> It looks like a bug to me and your suggested fix looks appropriate. >>> I've applied this fix to svn/trunk and OSG-3.2 braches. >>> >>> Thanks, >>> Robert. >>> >>> On 16 June 2014 12:00, Samuel M <[email protected]> wrote: >>> > Hello, >>> > >>> > we were using the MergeGeometryVisitor for our geometry and lately >>> > realized >>> > that it totally ignores the setTargetMaximumNumberOfVertices settingf >>> > 10000 >>> > (or actually every setting). >>> > >>> > While trying to comprehend what bool >>> > Optimizer::MergeGeometryVisitor::mergeGeode(osg::Geode& geode) is trying >>> > to >>> > do I came across this code: >>> > >>> > unsigned int numVertices(duplicateList.front()->getVertexArray() ? >>> > duplicateList.front()->getVertexArray()->getNumElements() : 0); >>> > DuplicateList::iterator eachGeom(duplicateList.begin()+1); >>> > // until all geometries have been checked or >>> > _targetMaximumNumberOfVertices >>> > is reached >>> > for (;eachGeom!=duplicateList.end(); ++eachGeom) >>> > { >>> > unsigned int numAddVertices((*eachGeom)->getVertexArray() ? >>> > (*eachGeom)->getVertexArray()->getNumElements() : 0); >>> > if (numVertices+numAddVertices<_targetMaximumNumberOfVertices) >>> > { >>> > break; >>> > } >>> > else >>> > { >>> > numVertices += numAddVertices; >>> > } >>> > } >>> > >>> > // push back if bellow the limit >>> > if (numVertices<_targetMaximumNumberOfVertices) >>> > { >>> > if (duplicateList.size()>1) needToDoMerge = true; >>> > mergeList.push_back(duplicateList); >>> > mergeListChecked.erase(itr); >>> > } >>> > >>> > >>> > This piece of code iterates over a given list of duplicates and fetches >>> > the >>> > vertex count of the first object. Then it iterates over second to last >>> > objects BUT for the second element it fetches numAddVertices and enters >>> > the >>> > given condition: >>> > if (numVertices+numAddVertices<_targetMaximumNumberOfVertices) >>> > >>> > Our scenario has 3 vertices in first and 3 vertices in second geometry >>> > which >>> > result in 3+3 < 100000. So the code directly breaks. >>> > Then it checks again for numVertices < targetMaximumNumberOfVertices >>> > which >>> > is still true and then PUSHES the entire list into mergeList. >>> > >>> > Our example is a flight file consisting of thousands of geodes having >>> > only 3 >>> > vertcies. The duplicateList has several thousand elements but the code >>> > checks only the first two and then pushes the first two AND the >>> > remaining >>> > thousands of geodes into the list to merge exceeding the >>> > _targetMaximumNumberOfVertices. >>> > >>> > >>> > Shouldn't the condition >>> > if (numVertices+numAddVertices<_targetMaximumNumberOfVertices) >>> > be >>> > if (numVertices+numAddVertices>_targetMaximumNumberOfVertices) >>> > >>> > and the mergeList.push_back(duplicateList); >>> > pushing only second to eachGeom range into the list? >>> > >>> > Regards, >>> > Sam >>> > >>> > _______________________________________________ >>> > osg-users mailing list >>> > [email protected] >>> > >>> > http://lists.openscenegraph.org/listinfo.cgi/osg-users-openscenegraph.org >>> > >>> _______________________________________________ >>> osg-users mailing list >>> [email protected] >>> http://lists.openscenegraph.org/listinfo.cgi/osg-users-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
