Fix Ogre2 assertions during ray queries

[darksylinc]

Summary

This bug had been annoying me for months for which I silenced those asserts.

But given that I need to implement SCENE_STATIC to get global illumination going, it is dangerous to ignore those asserts so I had to fix this.

ign-rendering would assert (inside a debug build of Ogre2) every time the mouse hovers the GUI.

This PR changes both API and ABI. Technically speaking what ign-rendering was doing was wrong but it didn't seem to matter too much (if at all); which is why I don't care to break API/ABI without backporting.

If this fix makes it into Fortress then great, otherwise it's not a big deal. Ideally ogre2 fork should include this fix we added to 2.3 and that I just backported to 2.2; but again no rush.

codecheck

Complains:

/home/matias/Projects/ign/src/ign-rendering/include/ignition/rendering/base/BaseRayQuery.hh:146: Using C-style cast. Use static_cast(...) instead [readability/casting] [4]

It seems it doesn't like the argument getting commented out:

RayQueryResult BaseRayQuery<T>::ClosestPoint(bool /*_forceSceneUpdate*/) {} // complains
RayQueryResult BaseRayQuery<T>::ClosestPoint(bool _forceSceneUpdate) {} // does not complain

However doing so will cause unused argument warning during build.
It may be possible to silence both using an UNUSED() macro if it doesn't exist already:

#define IGN_UNUSED(x) do { (void)sizeof(x); } while(0)

RayQueryResult BaseRayQuery<T>::ClosestPoint(bool _forceSceneUpdate)
{
  IGN_UNUSED(_forceSceneUpdate);
}

// Alternatively C++17
RayQueryResult BaseRayQuery<T>::ClosestPoint([[maybe_unused]] bool _forceSceneUpdate) {}

// Attributes on GCC/Clang
RayQueryResult BaseRayQuery<T>::ClosestPoint(__attribute__((unused)) bool _forceSceneUpdate) {}

// It may be best to use a macro to abstract these changes:
RayQueryResult BaseRayQuery<T>::ClosestPoint(IGN_UNUSED_ARG bool _forceSceneUpdate) {}

Commit msg

This change negatively affects performance as we now force a scene
update with each query.

Users can specify ClosestPoint(false) if they need to call this function
multiple times without modifying anything or if they know the scene is
up to date

Signed-off-by: Matias N. Goldberg [email protected]

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

[ahcorde]

  /github/workspace/include/ignition/rendering/base/BaseRayQuery.hh:146:  Using C-style cast.  Use static_cast<bool>(...) instead  [readability/casting] [4]
  /github/workspace/ogre/src/OgreRayQuery.cc:63:  Using C-style cast.  Use static_cast<bool>(...) instead  [readability/casting] [4]

[darksylinc]

  /github/workspace/include/ignition/rendering/base/BaseRayQuery.hh:146:  Using C-style cast.  Use static_cast<bool>(...) instead  [readability/casting] [4]
  /github/workspace/ogre/src/OgreRayQuery.cc:63:  Using C-style cast.  Use static_cast<bool>(...) instead  [readability/casting] [4]

Hi please read the post above. I need guidance on which solution you want to fix this false positive from codecheck

[ahcorde]

  /github/workspace/include/ignition/rendering/base/BaseRayQuery.hh:146:  Using C-style cast.  Use static_cast<bool>(...) instead  [readability/casting] [4]
  /github/workspace/ogre/src/OgreRayQuery.cc:63:  Using C-style cast.  Use static_cast<bool>(...) instead  [readability/casting] [4]

Hi please read the post above. I need guidance on which solution you want to fix this false positive from codecheck

I would say that adding // NOLINT will fix this false positives

[chapulina]

LGTM, we just need to make sure the linters are happy before merging.

[darksylinc]

LGTM, we just need to make sure the linters are happy before merging.

I haven't got the time yet to fix it, but I haven't forgotten about this PR. Sorry it's taking so long.

[darksylinc]

OK I've rebased and updated the PR to latest main.

Now we're waiting on the CI. Crossing fingers it works out 🤞

[codecov]

Codecov Report

Merging #415 (98d825f) into main (7f34b67) will increase coverage by 0.00%.
The diff coverage is 71.42%.

@@           Coverage Diff           @@
##             main     #415   +/-   ##
=======================================
  Coverage   53.46%   53.47%           
=======================================
  Files         199      199           
  Lines       19743    19745    +2     
=======================================
+ Hits        10556    10558    +2     
  Misses       9187     9187           

Impacted Files	Coverage Δ
include/ignition/rendering/RayQuery.hh	60.00% <ø> (ø)
include/ignition/rendering/base/BaseRayQuery.hh	36.84% <0.00%> (ø)
ogre/src/OgreRayQuery.cc	0.00% <0.00%> (ø)
ogre2/src/Ogre2RayQuery.cc	41.00% <100.00%> (+1.20%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f34b67...98d825f. Read the comment docs.