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 

PrimitiveSet.cpp correction


 
Post new topic   Reply to topic    OpenSceneGraph Forum Forum Index -> Submission
View previous topic :: View next topic  
Author Message
mp3butcher (Julien Valentin)
Appreciator


Joined: 17 Feb 2010
Posts: 520
Location: France

PostPosted: Mon Jun 20, 2016 4:50 pm    Post subject:
PrimitiveSet.cpp correction
Reply with quote

Hi,

not really a bug but an incoherence:
if ebo is NULL it can't be bound as IndexArray (bindElementBufferObject call ebo->isdirty without any check)

Thank you!

Cheers,
Julien
Back to top
View user's profile Send private message Visit poster's website
robertosfield
OSG Project Lead


Joined: 18 Mar 2009
Posts: 12321

PostPosted: Tue Jun 21, 2016 8:10 am    Post subject:
PrimitiveSet.cpp correction
Reply with quote

Hi Julian,

I have just reviewed the code and understand the logic of the change,
just looking at the local code it's what would seem the appropriate
implementation. However, the original code is probably more robust.
If you have two adjacent PrimitiveSet within the same osg::Geometry
but didn't all use EBO then you'd end up with an EBO being still bound
when it shouldn’t be. I realise this case won't be common and will
generally be undesirable it's a condition that won't be able to
enforce without adding more checks elsewhere.

Since one will normally use VBO and EBO at the same time the code path
that the CPU will take will essentially do all the same operations in
both the original code and your proposed change, so for normal usage
it won't be any more efficient. Given there is a possibility of a bug
appearing due to the lack of unbinding between PrimitiveSet I don't
feel it would be appropriate to adopt it.

Robert.

Robert.

On 20 June 2016 at 17:50, Julien Valentin <> wrote:
Quote:
Hi,

not really a bug but an incoherence:
if ebo is NULL it can't be bound as IndexArray (bindElementBufferObject call ebo->isdirty without any check)

Thank you!

Cheers,
Julien

------------------
Read this topic online here:
http://forum.openscenegraph.org/viewtopic.php?p=67702#67702








------------------
Post generated by Mail2Forum
Back to top
View user's profile Send private message
mp3butcher (Julien Valentin)
Appreciator


Joined: 17 Feb 2010
Posts: 520
Location: France

PostPosted: Tue Jun 21, 2016 10:02 am    Post subject:
Re: PrimitiveSet.cpp correction
Reply with quote

is adding unbindElementBufferObject

robertosfield wrote:
Hi Julian,

I have just reviewed the code and understand the logic of the change,
just looking at the local code it's what would seem the appropriate
implementation. However, the original code is probably more robust.
If you have two adjacent PrimitiveSet within the same osg::Geometry
but didn't all use EBO then you'd end up with an EBO being still bound
when it shouldn’t be. I realise this case won't be common and will
generally be undesirable it's a condition that won't be able to
enforce without adding more checks elsewhere.


I don't understand the use case you described.
When you're writing about primset that doesn't use EBO, do you refer to DrawArrays?
Because if you refer to DrawElements that doesn't use EBO (the else path in "DrawElementsXX::draw's if(ebo)" ) the current code would crash before reaching it ( on state.bindElementBufferObject(ebo) );
And if you're refering as I expect to DrawArrays the fact that there's EBO currently bound there isn't any incidence as the drawArrays call will simply ignore it...?!

So to stay simple how can to else path in "DrawElementsXX::draw's if(ebo)" could ever be reached without crashing on state.bindElementBufferObject(ebo) if the precedently drawn primset have ebo attached?

robertosfield wrote:


Since one will normally use VBO and EBO at the same time the code path
that the CPU will take will essentially do all the same operations in
both the original code and your proposed change, so for normal usage
it won't be any more efficient. Given there is a possibility of a bug
appearing due to the lack of unbinding between PrimitiveSet I don't
feel it would be appropriate to adopt it.
Back to top
View user's profile Send private message Visit poster's website
mp3butcher (Julien Valentin)
Appreciator


Joined: 17 Feb 2010
Posts: 520
Location: France

PostPosted: Tue Jun 21, 2016 10:07 am    Post subject:
Re: PrimitiveSet.cpp correction
Reply with quote

Oups forget what I tell about my simpification as "else path in DrawElementsXX::draw's if(ebo)" may only be reached when compiling DisplayLists....

mp3butcher wrote:

robertosfield wrote:
Hi Julian,

I have just reviewed the code and understand the logic of the change,
just looking at the local code it's what would seem the appropriate
implementation. However, the original code is probably more robust.
If you have two adjacent PrimitiveSet within the same osg::Geometry
but didn't all use EBO then you'd end up with an EBO being still bound
when it shouldn’t be. I realise this case won't be common and will
generally be undesirable it's a condition that won't be able to
enforce without adding more checks elsewhere.


I don't understand the use case you described.
When you're writing about primset that doesn't use EBO, do you refer to DrawArrays?
Because if you refer to DrawElements that doesn't use EBO (the else path in "DrawElementsXX::draw's if(ebo)" ) the current code would crash before reaching it ( on state.bindElementBufferObject(ebo) );
And if you're refering as I expect to DrawArrays the fact that there's EBO currently bound there isn't any incidence as the drawArrays call will simply ignore it...?!

So to stay simple how can to else path in "DrawElementsXX::draw's if(ebo)" could ever be reached without crashing on state.bindElementBufferObject(ebo) if the precedently drawn primset have ebo attached?

robertosfield wrote:


Since one will normally use VBO and EBO at the same time the code path
that the CPU will take will essentially do all the same operations in
both the original code and your proposed change, so for normal usage
it won't be any more efficient. Given there is a possibility of a bug
appearing due to the lack of unbinding between PrimitiveSet I don't
feel it would be appropriate to adopt it.
Back to top
View user's profile Send private message Visit poster's website
mp3butcher (Julien Valentin)
Appreciator


Joined: 17 Feb 2010
Posts: 520
Location: France

PostPosted: Tue Jun 21, 2016 10:15 am    Post subject:
Re: PrimitiveSet.cpp correction
Reply with quote

No don't forget it in fact I'm wrong to believe im wrong
mp3butcher wrote:
Oups forget what I tell about my simpification as "else path in DrawElementsXX::draw's if(ebo)" may only be reached when compiling DisplayLists....

mp3butcher wrote:

robertosfield wrote:
Hi Julian,

I have just reviewed the code and understand the logic of the change,
just looking at the local code it's what would seem the appropriate
implementation. However, the original code is probably more robust.
If you have two adjacent PrimitiveSet within the same osg::Geometry
but didn't all use EBO then you'd end up with an EBO being still bound
when it shouldn’t be. I realise this case won't be common and will
generally be undesirable it's a condition that won't be able to
enforce without adding more checks elsewhere.


I don't understand the use case you described.
When you're writing about primset that doesn't use EBO, do you refer to DrawArrays?
Because if you refer to DrawElements that doesn't use EBO (the else path in "DrawElementsXX::draw's if(ebo)" ) the current code would crash before reaching it ( on state.bindElementBufferObject(ebo) );
And if you're refering as I expect to DrawArrays the fact that there's EBO currently bound there isn't any incidence as the drawArrays call will simply ignore it...?!

So to stay simple how can to else path in "DrawElementsXX::draw's if(ebo)" could ever be reached without crashing on state.bindElementBufferObject(ebo) if the precedently drawn primset have ebo attached?

robertosfield wrote:


Since one will normally use VBO and EBO at the same time the code path
that the CPU will take will essentially do all the same operations in
both the original code and your proposed change, so for normal usage
it won't be any more efficient. Given there is a possibility of a bug
appearing due to the lack of unbinding between PrimitiveSet I don't
feel it would be appropriate to adopt it.
Back to top
View user's profile Send private message Visit poster's website
mp3butcher (Julien Valentin)
Appreciator


Joined: 17 Feb 2010
Posts: 520
Location: France

PostPosted: Tue Jun 21, 2016 10:36 am    Post subject:
Re: PrimitiveSet.cpp correction
Reply with quote

In all case I tested your usecase in all possibles senses and couldn't find what the problem could be...

mp3butcher wrote:
No don't forget it in fact I'm wrong to believe im wrong
mp3butcher wrote:
Oups forget what I tell about my simpification as "else path in DrawElementsXX::draw's if(ebo)" may only be reached when compiling DisplayLists....

mp3butcher wrote:

robertosfield wrote:
Hi Julian,

I have just reviewed the code and understand the logic of the change,
just looking at the local code it's what would seem the appropriate
implementation. However, the original code is probably more robust.
If you have two adjacent PrimitiveSet within the same osg::Geometry
but didn't all use EBO then you'd end up with an EBO being still bound
when it shouldn’t be. I realise this case won't be common and will
generally be undesirable it's a condition that won't be able to
enforce without adding more checks elsewhere.


I don't understand the use case you described.
When you're writing about primset that doesn't use EBO, do you refer to DrawArrays?
Because if you refer to DrawElements that doesn't use EBO (the else path in "DrawElementsXX::draw's if(ebo)" ) the current code would crash before reaching it ( on state.bindElementBufferObject(ebo) );
And if you're refering as I expect to DrawArrays the fact that there's EBO currently bound there isn't any incidence as the drawArrays call will simply ignore it...?!

So to stay simple how can to else path in "DrawElementsXX::draw's if(ebo)" could ever be reached without crashing on state.bindElementBufferObject(ebo) if the precedently drawn primset have ebo attached?

robertosfield wrote:


Since one will normally use VBO and EBO at the same time the code path
that the CPU will take will essentially do all the same operations in
both the original code and your proposed change, so for normal usage
it won't be any more efficient. Given there is a possibility of a bug
appearing due to the lack of unbinding between PrimitiveSet I don't
feel it would be appropriate to adopt it.
Back to top
View user's profile Send private message Visit poster's website
mp3butcher (Julien Valentin)
Appreciator


Joined: 17 Feb 2010
Posts: 520
Location: France

PostPosted: Tue Jun 21, 2016 11:02 am    Post subject:
Re: PrimitiveSet.cpp correction
Reply with quote

The only thing I can ensure is the current code fails if you call setUseVertexBufferObject(true) during runtime (on a geometry that originally use DisplayLists)



mp3butcher wrote:
In all case I tested your usecase in all possibles senses and couldn't find what the problem could be...

mp3butcher wrote:
No don't forget it in fact I'm wrong to believe im wrong
mp3butcher wrote:
Oups forget what I tell about my simpification as "else path in DrawElementsXX::draw's if(ebo)" may only be reached when compiling DisplayLists....

mp3butcher wrote:

robertosfield wrote:
Hi Julian,

I have just reviewed the code and understand the logic of the change,
just looking at the local code it's what would seem the appropriate
implementation. However, the original code is probably more robust.
If you have two adjacent PrimitiveSet within the same osg::Geometry
but didn't all use EBO then you'd end up with an EBO being still bound
when it shouldn’t be. I realise this case won't be common and will
generally be undesirable it's a condition that won't be able to
enforce without adding more checks elsewhere.


I don't understand the use case you described.
When you're writing about primset that doesn't use EBO, do you refer to DrawArrays?
Because if you refer to DrawElements that doesn't use EBO (the else path in "DrawElementsXX::draw's if(ebo)" ) the current code would crash before reaching it ( on state.bindElementBufferObject(ebo) );
And if you're refering as I expect to DrawArrays the fact that there's EBO currently bound there isn't any incidence as the drawArrays call will simply ignore it...?!

So to stay simple how can to else path in "DrawElementsXX::draw's if(ebo)" could ever be reached without crashing on state.bindElementBufferObject(ebo) if the precedently drawn primset have ebo attached?

robertosfield wrote:


Since one will normally use VBO and EBO at the same time the code path
that the CPU will take will essentially do all the same operations in
both the original code and your proposed change, so for normal usage
it won't be any more efficient. Given there is a possibility of a bug
appearing due to the lack of unbinding between PrimitiveSet I don't
feel it would be appropriate to adopt it.
Back to top
View user's profile Send private message Visit poster's website
mp3butcher (Julien Valentin)
Appreciator


Joined: 17 Feb 2010
Posts: 520
Location: France

PostPosted: Tue Jun 21, 2016 11:17 am    Post subject:
Re: PrimitiveSet.cpp correction
Reply with quote

Hi Robert
I will prepare a submission for Geometry changes in order to overcome this issue.
Sorry for my repeated posts

mp3butcher wrote:
The only thing I can ensure is the current code fails if you call setUseVertexBufferObject(true) during runtime (on a geometry that originally use DisplayLists)



mp3butcher wrote:
In all case I tested your usecase in all possibles senses and couldn't find what the problem could be...

mp3butcher wrote:
No don't forget it in fact I'm wrong to believe im wrong
mp3butcher wrote:
Oups forget what I tell about my simpification as "else path in DrawElementsXX::draw's if(ebo)" may only be reached when compiling DisplayLists....

mp3butcher wrote:

robertosfield wrote:
Hi Julian,

I have just reviewed the code and understand the logic of the change,
just looking at the local code it's what would seem the appropriate
implementation. However, the original code is probably more robust.
If you have two adjacent PrimitiveSet within the same osg::Geometry
but didn't all use EBO then you'd end up with an EBO being still bound
when it shouldn’t be. I realise this case won't be common and will
generally be undesirable it's a condition that won't be able to
enforce without adding more checks elsewhere.


I don't understand the use case you described.
When you're writing about primset that doesn't use EBO, do you refer to DrawArrays?
Because if you refer to DrawElements that doesn't use EBO (the else path in "DrawElementsXX::draw's if(ebo)" ) the current code would crash before reaching it ( on state.bindElementBufferObject(ebo) );
And if you're refering as I expect to DrawArrays the fact that there's EBO currently bound there isn't any incidence as the drawArrays call will simply ignore it...?!

So to stay simple how can to else path in "DrawElementsXX::draw's if(ebo)" could ever be reached without crashing on state.bindElementBufferObject(ebo) if the precedently drawn primset have ebo attached?

robertosfield wrote:


Since one will normally use VBO and EBO at the same time the code path
that the CPU will take will essentially do all the same operations in
both the original code and your proposed change, so for normal usage
it won't be any more efficient. Given there is a possibility of a bug
appearing due to the lack of unbinding between PrimitiveSet I don't
feel it would be appropriate to adopt it.
Back to top
View user's profile Send private message Visit poster's website
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 Converting PrimitiveSet to use triang... Waaayoff General 7 Fri Jul 27, 2018 12:06 pm View latest post
No new posts Adding primitiveset in runtime sergio2k18 General 7 Tue May 01, 2018 9:12 pm View latest post
No new posts Visual Studio 2013 - PrimitiveSet.cpp... Voerman, L. Submission 1 Mon Feb 01, 2016 3:42 pm View latest post
No new posts bug in primitiveset.cpp? mp3butcher General 2 Mon Sep 07, 2015 5:45 pm View latest post
No new posts missing PrimitiveSet UniqueID impleme... robertosfield Submission 0 Fri Jun 28, 2013 4:16 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