OpenSceneGraph Forum Forum Index OpenSceneGraph Forum
Official forum which mirrors the existent OSG mailing lists. Messages posted here are forwarded to the mailing list and vice versa.
 
   FAQFAQ    SearchSearch    MemberlistMemberlist    RulesRules    UsergroupsUsergroups    RegisterRegister 
 Mail2Forum SettingsMail2Forum Settings  ProfileProfile   Log in to check your private messagesLog in to check your private messages   Log inLog in 
   AlbumAlbum  OpenSceneGraph IRC ChatOpenSceneGraph IRC Chat   SmartFeedSmartFeed 

Submission Further patch for: MergeGeometryVisitor behaviour, is this a bug?


 
Post new topic   Reply to topic    OpenSceneGraph Forum Forum Index -> Submission
View previous topic :: View next topic  
Author Message
Samuel M
Guest





PostPosted: Tue Jun 17, 2014 6:49 am    Post subject:
Submission Further patch for: MergeGeometryVisitor behaviour, is this a bug?
Reply with quote

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.


Quote:
Date: Mon, 16 Jun 2014 17:19:20 +0100
From:
To:
Subject: Re: 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 <> wrote:
Quote:
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






------------------
Post generated by Mail2Forum
Back to top
robertosfield
OSG Project Lead


Joined: 18 Mar 2009
Posts: 12144

PostPosted: Tue Jun 17, 2014 9:23 am    Post subject:
Submission Further patch for: MergeGeometryVisitor behaviour, is this a bug?
Reply with quote

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 <> wrote:
Quote:

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.


Quote:
Date: Mon, 16 Jun 2014 17:19:20 +0100
From:
To:
Subject: Re: 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 <> wrote:
Quote:
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









------------------
Post generated by Mail2Forum
Back to top
View user's profile Send private message
robertosfield
OSG Project Lead


Joined: 18 Mar 2009
Posts: 12144

PostPosted: Wed Jun 25, 2014 10:58 am    Post subject:
Submission Further patch for: MergeGeometryVisitor behaviour, is this a bug?
Reply with quote

No reply so have gone ahead and merged without tagging a specific name.

On 17 June 2014 10:22, Robert Osfield <> wrote:
Quote:
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 <> wrote:
Quote:

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.


Quote:
Date: Mon, 16 Jun 2014 17:19:20 +0100
From:
To:
Subject: Re: 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 <> wrote:
Quote:
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









------------------
Post generated by Mail2Forum
Back to top
View user's profile Send private message
Display posts from previous:   
Post new topic   Reply to topic    OpenSceneGraph Forum Forum Index -> Submission All times are GMT
Page 1 of 1

 
Jump to:  
You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot vote in polls in this forum
You cannot attach files in this forum
You cannot download files in this forum

Similar Topics
Topic Author Forum Replies Posted
No new posts strange behaviour with osgText::Text christoph General 0 Wed Jul 18, 2018 12:06 pm View latest post
No new posts Strange behaviour on GBufferCamera wi... wernerM General 5 Wed Jun 27, 2018 12:53 pm View latest post
No new posts Strange behaviour - no explanation wernerM General 1 Wed Jan 24, 2018 5:08 pm View latest post
No new posts MergeGeometryVisitor Andreas Ekstrand Submission 1 Fri Nov 24, 2017 3:27 pm View latest post
No new posts missed submission? 3rdParty Package -... zonk Submission 6 Fri Apr 07, 2017 10:13 pm View latest post


Board Security Anti Bot Question MOD - phpBB MOD against Spam Bots
Powered by phpBB © 2001, 2005 phpBB Group
Protected by Anti-Spam ACP