VDBObject : Don't bake in automatic metadata when querying metadata

[danieldresser-ie]

This addresses proposal 2) from John's comment here:
GafferHQ/gaffer#5923 (comment)

I think a PR is probably the most direct way to discuss it, though I'm not entirely convinced that just merging this is necessarily correct.

I think the argument is completely sound that the previous behaviour was wrong - we were calling this function assuming it was const, and this was resulting in things getting written in the cache in a completely non-threadsafe way. This definitely seems more correct.

But just doing this results in a worse experience for users. Seeing the voxel counts and bounding box sizes in the Gaffer Scene Inspector seems quite useful.

Perhaps the Gaffer inspector should compute the bounding box and voxel counts explicitly, in addition to showing the metadata? That is pretty independent from this, but maybe we should have a plan for it before we merge this.

[danieldresser-ie]

Added a fix commit so that worldBound returns correct results in the scenario that the metadata isn't set.

This kind of confirms though that finding the right approach for all this stuff may not be trivial. evalActiveVoxelBoundingBox seems potentially expensive to possibly end up calling repeatedly on the same VDB every time you need the bbox ... and it only works if the grids are fully loaded?

I do start to wonder whether there could be merit in the opposite approach ( make sure that everything that accesses the metadata goes through a wrapper that is thread safe and adds in the standard stats if they aren't present ).

[johnhaddon]

Recap on some changes since this PR was opened :

• Gaffer's SphereLevelSet no longer adds stats metadata, bringing it into line with Gaffer's other VDB nodes : SphereLevelSet : Don't add standard stats to the metadata GafferHQ/gaffer#5928.
• Gaffer's SphereLevelSet now has an efficient bound implementation that doesn't rely on VDBObject::bound() : SphereLevelSet : Add more efficient overrides for hash/computeBound() GafferHQ/gaffer#6537.
• Gaffer's SceneInspector has been revamped to show bounding box, active voxel count etc independently of the metadata (which may be out of sync). This is done on a background thread and the results are cached : SceneInspector Revamp GafferHQ/gaffer#6515.
• Gaffer's SceneInspector still shows metadata, but gets it direct from the Grid, instead of using VDBObject::metadata().

For those reasons, I propose to update this PR to go even further, and remove VDBObject::metadata() entirely. If VDBObject is the thinnest wrapper we can make it, then we have less opportunities to create the kind of bug this PR is addressing.

Any thoughts @danieldresser-ie?

[danieldresser-ie]

If Gaffer isn't using VDBObject::metadata(), then it seems like a pretty obvious win from our point of view to remove it.

I suppose the only question is whether anyone other than Gaffer is using this, but that does't seem particularly likely.

[johnhaddon]

@ivanimanishi, any objections to removing the VDBObject::metadata() method?

[ivanimanishi]

We do have an IERendering node (VolumeMetadataToAttributes) that uses that method (IECore::ConstCompoundObjectPtr metadata = const_cast<IECoreVDB::VDBObject *>( vdbObject )->metadata( gridName );), and that is in active use.

I could find it in a single box though (which is itself used extensively in production), so if there are alternative approaches to address the production needs, it might not be too hard to update things.

So, on initial inspection, we do need to keep the metadata() method.

If there is an easy alternative in c++ to access the same data skipping cortex (maybe by accessing VDB's api directly?) to replace that line I linked above, we could update VolumeMetadataToAttributes to use it.

Or, if you want to understand how it's being used in production to see if there is a better approach they can use, reach out to Martin Bohn, as he's the author of said Box that uses VolumeMetadataToAttributes.

An final extra piece of info that might be useful to you, as far as I can tell, the properties from the metadata currently being queried are: file_voxel_count, maximum, file_bbox_max and file_bbox_min.

[johnhaddon]

If there is an easy alternative in c++ to access the same data skipping cortex (maybe by accessing VDB's api directly?) to replace that line I linked above, we could update VolumeMetadataToAttributes to use it.

We're using the VDB API directly in Gaffer, and it's pretty straightforward :

https://github.com/GafferHQ/gaffer/blob/main/src/GafferVDBUIModule/GafferVDBUIModule.cpp#L88-L150

If you think you can switch over too then let us know, but if you have enough on your plate already then we can keep that method around for a bit longer...

[ivanimanishi]

Let's keep it around for a bit longer, since we already have to deal with a quite a few other breaking changes.

[danieldresser-ie]

Updated this PR to keep the worldBound fix, but not change metadata() other than adding a deprecation warning. I'm fairly hopeful that the place where ImageEngine is relying on metadata() can be removed completely in the long term, but it makes sense for now to not change something that isn't broken.