Another few fixes for OcclusionQueryNode #706
Conversation
In the case of an invalid geometry there're two ways to react on it, traverse the subgraph or not traversing it. In the case of an invalid query box from e.g. a geometry with a cleared DrawElements list, not traversing the subgraph would be the right thing. But if the geometry only consists of e.g. one point, then the query box would be always invalid and the subgraph never traversed. So it seems safer to always traverse the subgraph, which is done in this patch.
There're cases that the occlusion test result has been retrieved after the query geometry has been changed, it's the result of the geometry before the change.
|
I have done a first pass review but feel that it's really several PR's worth rolled into one. There is the proposed fix and then there is a whole heap of debug messages that might be appropriate for code that is being debugged but not necessarily appropriate for shipping code. Also the line where you use the nonConstThis to access a bool in a const way is unnecessary and confusing for the compiler/reader. Any time there is some extra code which doesn't do anything useful raises a red flag for what is actually indended by the user. The reason for the nonConstThis is that OSG's NodeVisitor only works with non const objects, so it's a wokraround for this, this hacky workaround should only be used for code that actually needs that workaround and not elsewhere so it's clearer to the developer is that why it's be used. |
|
There is the proposed fix and then there is a whole heap of debug messages that might be appropriate for code that is being debugged but not necessarily appropriate for shipping code.
I can‘t quite follow. OSG_DEBUG is for debugging purposes. So the added output just helps to understand why something has or hasn‘t been occluded. But I don‘t feel strong about the output, so I can just remove it.
Also the line where you use the nonConstThis to access a bool in a const way is unnecessary and confusing for the compiler/reader. Any time there is some extra code which doesn't do anything useful raises a red flag for what is actually indended by the user.
The nonConstThis is also used to update the query geometry. Updating only internal
state which isn‘t detectable from the outside is in my view fine.
Are you against the added _validQueryGeometry flag? If not, what is the implementation alternative?
|
|
If the _validQueryGeometry is modified within a const method then it should be declared mutable, and one should take care about whether there might be a race condition associated with it. Using the const cast to get around const behaviour should always be a last resort. |
2 participants
Hi Robert,
this solves the last flickering issues in our application.
Greetings,
Daniel