fix(Camera::isViewOutOfDate): Don't update transformations

[jmachowinski]

Until this patch isViewOutOfDate was updating parent transformations every time it was called. This looks like a bug, as the function itself is const, and the behavior is unexpected and not documented.

This patch replaces the force update function with the non update one.

Note, this may break existing code that relied on this bug.

Related to / Fixes #579

[darksylinc]

Does gazebo run fine after this change?

I'm asking because Gazebo is pretty strict about ABI compatibility. Basically see my question and concern here: https://chatgpt.com/share/6a3bf763-e86c-83e9-91ec-b3e911f5f7ac

(Note: ChatGPT suggested to use orientationDirty but AFAICT that would definitely break the ABI so it's not a valid solution).

[jmachowinski]

My local tests did not show any regressions, but in our case all cameras are static.

As far as I can tell, gazebo also seems to be pinned to v2.3.3 with some custom patches on top.

@azeey Any thoughts on the issue ?

[darksylinc]

This change likely low risk IMHO. I'll later run my suite of tests to see if we catch any regressions. If that passes there's 2 possible outcomes for gazebo:

1. Nothing bad happens. Free perf! Yay! Most likely outcome.
2. Minor (possible) to major (rare) rendering artifacts in some edge cases. If OgreNext is compiled in Debug mode, it would trigger an assert. Gazebo or the client app (if Gazebo was customized) would need to write a small fix and rebuild.

Like I said in the AI prompt, strictly speaking the API and ABI isn't broken; and the AI pointed out about the "contract", which I can clarify the "contract" was that the correct behavior is after this PR, not before.

Thus code written against OgreNext that implicitly depended on the behavior before the PR were technically broken in the first place.

Nonetheless, it all boils down to what users expect when they update patch versions (and how likely those edge cases are to trigger, assuming they happen at all).

[azeey]

I'll defer to @iche033 to check if the behavior change would be considered breaking or if we can account for it in gz-rendering.

But generally speaking, we've had discussions on what our API/ABI compatibility policy should be on upstream dependencies we distribute such as Ogre-next and DART and the conclusion we've come to is that we should only enforce API/ABI compatibility on the public API/ABI of the Gazebo libraries. If upstream dependencies are included in our public API, then we'd apply the same policy.

For Ogre-next, I believe everything is encapsulated in gz-rendering and no Gazebo library outside of gz-rendering diretly depends on Ogre-next. Therefore, we have the freedom tol update Ogre-next as necessary to get important fixes like this one without worrying about API/ABI compatibility.

That being said, there might be users that depend on the Ogre-next binary we distribute. If we introduce breaking changes, we should at least announce it on our comms channels before pushing the changes. I'd also encourage us to make breaking changes sparingly for their sake.

cc @j-rivero

[darksylinc]

I'll rephrase: On the slight chance that OgreNext causes a bug, it would need an update from Gazebo.

My understanding is that Gazebo what ideally wants is that if OgreNext gets updated from say 2.3.1 -> 2.3.2, then Gazebo keeps working as usual.

What Gazebo usually wants to avoid is that updating OgreNext 2.3.1 -> 2.3.2 would also require updating gz-rendering from (say) 6.0.0 -> 6.0.1.

Sometimes this is unavoidable. Even bigger projects like SDL cause breakage after patch updates that require an update on the app that uses the library. This is usually undesirable but not always avoidable. Perhaps a popular example of this is an innocent change in Windows 11 exposing a GTA San Andreas bug 20 years later.

[jmachowinski]

I'll rephrase: On the slight chance that OgreNext causes a bug, it would need an update from Gazebo.

@darksylinc I got the feeling you are missing the vital peace of information, that @azeey is the gazebo project leader.

[darksylinc]

I got the feeling you are missing the vital peace of information, that @azeey is the gazebo project leader.

I didn't know 😄

But my comment was because I got the feeling Azeey was worrying about "what about ABI compatibility with other unrelated projects installed on (an Ubuntu) system?" while I was always strictly concerned about Gazebo itself.

[iche033]

I'll defer to @iche033 to check if the behavior change would be considered breaking or if we can account for it in gz-rendering.

There are some places that could potential be affected in gz-rendering. These are places where we do not guarantee that we update the ogre's scene graph / camera transform before querying the ogre's camera view matrix:

• ray queries , and
• 3D->2D Projection (which calls eventually calls getViewMatrix):

There maybe others. But for cases like these, we can fix them by doing an updateFromParent call in gz-rendering where necessary. We could also run our tests against this change and catch issues if possible. There is a chance that our test suite does not have test cases for these scenarios. The other concern is what @azeey mentioned, we could break other users who are relying on our ogre-next debs.

[darksylinc]

I've tested the PR and it did not pass Unit Tests.

Further fixes were required to pass them. These fixes were incorporated to branch 'master' (4.0) already and it should be possible to backport them to 2.3 / 3.0 without breaking ABI:

3b7bc94

My local tests did not show any regressions, but in our case all cameras are static.

I recommend you to test with a debug build of OgreNext, which has asserts enabled.

This PR triggered asserts when Forward Clustered was enabled. The actual reason for the assert was harmless (it wouldn't cause artifacts), but it tells me your tests did not include a debug version.

[azeey]

I'm agreeing with you @darksylinc. To be clear, I was saying we would distribute ogre-next debs that break API/ABI if the change is deemed critical. In the past, there might have been a Gazebo policy that forbids distributing such changes, but we have since decided that our API/ABI stability policy should be limited to Gazebo libraries (e.g. gz-rendering), not dependencies (e.g. ogre-next).

[jmachowinski]

@azeey In our use case (with 2 depth camera renderings) we see a 10% performance impact.
For us it is not critical that we merge this patch up stream, as we run our own build of gazebo / ogre anyway now.

I'll leave the decision up to you if you deem the performance gain important enough to update.

As there is already a mainline patch that fixes this issue, I am closing this PR now.

Thx again @darksylinc