danieldresser-ie
commented
2 months ago 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 ).
This addresses proposal 2) from John's comment here: https://github.com/GafferHQ/gaffer/pull/5923#issuecomment-2191354486
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.