OcclusionQueryNode: fix use case of user defined query geometry

[dan-t]

The user defined query geometry handling has been broken in several ways.

The previous way of defining a query geometry was using the non const
getQueryGeometry method and overriding its members. But then
OcclusionQueryNode wasn't aware of the geometry change and couldn't
internally handle it correctly.

The computeBound method never considered a user defined query geometry and
always just overrode the vertices of the geometry.

The member _validQueryGeometry wasn't correctly set.

This change should fix all this issues by introducing a small backward
compatibility break. The non const getQueryGeometry method is removed
forcing the user to use the setQueryGeometry method. But then OcclusionQueryNode
is aware of the user defined query geometry and can handle it correctly.

[openscenegraph]

@mp3butcher Julian, could you review this PR and see if it's addresses the issues you were up against?

[mp3butcher]

a pr against 3.6 would be better in order to review

[mp3butcher]

@robertosfield ...master lacks a PR i've done to take into acount _enable

cfd190a
and
5311d6f
may be merge in master
before this one

[mp3butcher]

@dan-t removing not const getQuerYGeometry breaks backward client usage:
client code may use this method to do custom geometry...further as getNumpixels is not const it mandates to use nonconst getQueryGeometry to retrieve visible pixels for custom decision based on num visible pixels

[dan-t]

On Wed, Aug 21, 2019 at 07:00:49AM -0700, Julien Valentin wrote: @dan-t removing not const getQuerYGeometry breaks backward client usage: client code may use this method to do custom geometry...

Have you read my detailed commit message? Keeping it backward compatible means having a broken version that's very error prone to use. If `_validQueryGeometry` isn't set to true, then even the occlusion query is never applied and the user will not be aware of it! My change forces the user to apply a minimal change to his code, but then it should just work.

further as getNumpixels is not const it mandates to use nonconst getQueryGeometry to retrieve visible pixels for custom decision based on num visible pixels

Then `getNumPixels` should be made const.

[dan-t]

@robertosfield Can you please decide if breaking backward compatibility is acceptable if it fixes a broken version.

[mp3butcher]

@dan-t a simple way to circumvent the issue would be to control query geometry validity in getPassed itself
something like

 if ( !_validQueryGeometry )
    {
        if(qg->getBoundingBox().valid())
            _validQueryGeometry = true;
        else
        {
            // 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.
            qg->reset();
    
            // The box of the query geometry is invalid, return false to not traverse
            // the subgraphs.
            _passed = false;
            return _passed;
        }
    }

[openscenegraph]

Hi Dan, I'm happy to see a breaking change if it addresses problem usage cases. I expect only a small % of users will be using occlussion query and those who do will sure want to have their usage working correctly. Cheers, Robert.

…

On Wed, 21 Aug 2019 at 15:40, Daniel Trstenjak ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/osg/OcclusionQueryNode <#813 (comment)> : > @@ -158,8 +158,11 @@ public: osg::StateSet* getQueryStateSet(); const osg::StateSet* getQueryStateSet() const; - // Get the QueryGeometry object used for occlusion query. Returns 0 if no QueryGeometry is created. - osg::QueryGeometry* getQueryGeometry(); @robertosfield <https://github.com/robertosfield> Can you please decide if breaking backward compatibility is acceptable if it fixes a broken version. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#813?email_source=notifications&email_token=AABUA2Y4CYBE6FFHXJLHJW3QFVHUZA5CNFSM4ILS4OYKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCCHZGYY#discussion_r316223737>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABUA224V3MFK42SDK7MOJLQFVHUZANCNFSM4ILS4OYA> .

[mp3butcher]

1a9fcfc
solve the backward compat issue

[dan-t]

1a9fcfc solve the backward compat issue

I‘m sorry, but that‘s just a hack. When you keep the non const ˋgetQueryGeometryˋ, you can‘t fix ˋcomputeBoundˋ , it just overrides the vertices of the user define query geometry. It even isn‘t thread safe to change the query geometry, that‘s why ˋsetQueryGeometryInternalˋ locks the ˋ_computeBoundMutexˋ.

[mp3butcher]

oups u're right (validgeometry is not muted properly)...
I'm pretty ok with your changes but as long as your pr is not against 3.6 branch or(@robertosfield don't merge #772 with master) i won't be happy with it

[dan-t]

I'm pretty ok with your changes but as long as your pr is not against 3.6 branch ***@***.*** don't merge #772 with master) i won't be happy with it

For your testing purposes you can just ˋgit cherry-pickˋ the commits onto your local branch.

[mp3butcher]

as i already said cherry-pick this commit into 3.6 branch involve conflicts so for proper reviewing something should be done by you (pr against 3.6) or Robert (merging #772 in master)

[openscenegraph]

I've merged the || !_enabled change in 3.6 into master:

void OcclusionQueryNode::traverseQuery( const Camera* camera, NodeVisitor& nv )
{
if (!_validQueryGeometry || ! _enabled)
return;

I don't know how it ended up diverged from 3.6, one missing commit me thinks. I will now merge this PR as it look generally OK, but I'm @dan-t lots here as I don't have too much knowledge about this particular class and it's usage.

[openscenegraph]

Ack, now got a compile error with master, the above tweak of mine wasn't appropriate... now looking into how to resolve it.

[openscenegraph]

Hang my head in shame, I broke the build be inappropriate trying to match the 3.6 and master on this PR without spotting all the changes. Now fixed:

3a17fd3

[mp3butcher]

@openscenegraph please merge with 3.6 branch