SceneCache::hash() uses file name where available

[davidsminor]

SceneCache::hash() used to use a memory address. Ideally you'd want this method to give a stable result across different processes, so now we use the file name if we're reading a scene cache from a file, and fall back to a memory address if we're using something like a MemoryIndexedIO.

[johnhaddon]

I'd be nice to get this merged and then we can remove the explicit filename hashing from GafferScene::SceneReader. I think we can do better than this though - how about we try computing the basic hash (of filename or address) once on initialisation rather than hashing in the filename each time? I suspect that should be quicker, and this is definitely a spot where time is of the essence. We could also include the file modification time in this precomputed hash, and then remove the hashing of GafferScene::SceneReader::refreshCountPlug() as well.

[andrewkaufman]

Just to comment on removing GafferScene::SceneReader::refreshCountPlug() from the hash, it is quite nice to know for sure that when you push the refresh button, the data is being recomputed / reloaded. If we remove that from the hash, I take it that would no longer be the case, and a user would be stuck relying on file modification time to drive any reload, is that correct? I can easily see people getting frustrated that pushing the refresh button doesn't necessarily actually refresh anything...

[johnhaddon]

We already call SharedSceneInterfaces::clear() and propagate dirtiness when the refresh count is changed - so it will definitely result in the instantiation of a new SceneCache at the very least. Are there ways a file can be changed without affecting the modification time (genuine question)?

There are other downsides to the refreshCount being included in the hash - imagine two nodes reading the same file, but where one has had refreshCount hit more times than the other. They have different hashes, even though they read the same thing, and as a result we waste space in the cache.

I definitely think that removing refreshCount from the hash is the right thing to do. Perhaps if modification time turns out not to be an appropriate substitute, we can add some unique instance number via SceneCache::hash() instead? Because all SceneReaders acquire their SceneCache from SharedSceneInterfaces, this will avoid the situation where two otherwise identical nodes end up with different hashes.

[andrewkaufman]

I don't know if a file can be changed without affecting mod time... I hope not though. Maybe I've just been abusing the refresh plug in absence of a force-recompute option. Something to say "don't read from the cache for this one node this one time, but still write to the cache".

I guess we may as well give it a go the way you suggest, and see if I still feel that need anymore.

[johnhaddon]

What circumstance (other than the file being changed on disk) causes a forced recompute to come up with a different answer than a previous one? Is there something obvious I'm missing? I definitely think deferring all hashing to the SceneInterface is the right thing to do.

[andrewkaufman]

You're thinking about it too logically... people want to be able to force something and know that it's been forced. It doesn't matter if the computer is actually smarter, the button pusher wants to be the smartest.

[johnhaddon]

So, I guess for this one we need to decide a course of action on the following :

• Should we compute the basic hash (of the filename or the address) once upfront as I suggested, in the hope of improved speed?
• Should we include the modification time in the hash and avoid hashing refreshCount?
• Do we need to synchronise this with a gaffer release which removes the filename from SceneReader::hash()?

[andrewkaufman]

After rereading the comments, I'm not sure why we never finished this one off. @johnhaddon do you think having hi open is helpful or should we close?

[johnhaddon]

Let's keep it open as a reminder. When I do the AlembicScene I'll be pretty keen to remove the filename hashing from the SceneReader and defer entirely to the SceneInterface, so I can take a look at it all then.

[andrewkaufman]

I made a note on #29 so we don't forget about this, and we handle it at the same time as AlembicScene. Lets close the PR now we have that other reminder.