Update DisplayDriverServer and ClientDisplayDriver #1448
Conversation
There was a problem hiding this comment.
Thanks Linas!
The general approach here seems good - it's pleasing to see that you've found a way of dropping this into the existing setup with really pretty minimal changes to the code.
I've made a bunch of inline comments with small suggestions for improvements, but nothing that changes the overall strategy. In addition to those and adding some comprehensive tests, it would be good to document the new parameters somewhere, possibly in DisplayDriverServer.h.
Cheers...
John
Adds option to server and client driver to allow a 'merge' driver that shares a display driver to write to.
ivanimanishi
force-pushed
the
linasb_allowServerToMergeDrivers
branch
from
e69edc6 to
6104274
Compare
There was a problem hiding this comment.
Thanks for the update Linas. Nice to see the implementation looking even simpler after the last round of fixes. I do have my doubts about the current approach to testing though - I would prefer something more akin to what we discussed offline, whereby we actually check that the merged driver receives the correct data and is closed at the right time. See comments inline...
Cheers...
John
| @@ -70,6 +73,8 @@ IE_CORE_DEFINERUNTIMETYPED( DisplayDriverServer ); | |||
| namespace | |||
| { | |||
|
|
|||
| static DisplayDriverServer::MergeMap g_mergeMap; | |||
There was a problem hiding this comment.
I didn't spot this before, but are we happy that the merge map is shared across all servers like this, and there isn't a separate one per server? It seems like this forces the client to generate a really unique ID, rather than only one that is unique within the server being connected to.
There was a problem hiding this comment.
In this context is the server something that is setup and running per every instance of Gaffer or would it be shared across all instance of Gaffer on a work station?
There was a problem hiding this comment.
Each instance of Gaffer will have its own server. Actually each instance will have a server for the Catalogue and then another server for each ShaderView (although we wouldn't expect those to be using the merging functionality).
But looking at DisplayDriverServer as a library component, which could be used anywhere, it might be better for the map to be per-server rather than static.
There was a problem hiding this comment.
Would making it non-static be enough or do you want g_mergeMap to be a member variable of the DisplayDriverServer?
There was a problem hiding this comment.
I think logically it should probably be a member.
| @@ -118,6 +119,47 @@ def testPortRangeRegistry( self ) : | |||
| s2 = IECoreImage.DisplayDriverServer() | |||
| self.assertEqual( s2.portNumber(), 45021 ) | |||
|
|
|||
| def testMergeMap( self ) : | |||
There was a problem hiding this comment.
This approach to testing seems a little weak - there's nothing to show that the right data is sent successfully to the merged driver, or that the merged driver is closed at the right time. It also forces to expose the map publicly, which I think would be better left private. Can we beef this up by sending actual data and testing that it arrives in an ImageDisplayDriver appropriately?
There was a problem hiding this comment.
As much as I do agree that it is a little weak, I don't think it is the job of this test to make sure that the the image driver receives correct information since that is the job of the image writer test.
Here I'm only trying to make sure that the functionality of the merge map is adhered to. However, I do see your point that it's important to test that the image being sent over is closed correctly even if the code there hasn't changed.
I'll add similar tests as in testTransfer() in ImageDisplayDriverTest.py to make sure that the image is closed correctly and that data is being sent over.
There was a problem hiding this comment.
I don't think it is the job of this test to make sure that the the image driver receives correct information
We're adding functionality to route data from multiple client drivers into a single driver on the server. It seems pretty fundamental that we should test that the data actually arrives.
There was a problem hiding this comment.
Also we have internal reasons at IE for exposing the map publicly
There was a problem hiding this comment.
What are the reasons? My reason for not wanting it to be public is that once it's in the API, it ties our hands in terms of future maintenance.
There was a problem hiding this comment.
We wanted a way of checking which version of Cortex we're using. So querying if the function exists, but we could do something else.
ivanimanishi
force-pushed
the
linasb_allowServerToMergeDrivers
branch
from
419c382 to
f0a8f9f
Compare
Adds option to server and client driver to allow a 'merge' driver that shares a display driver to write to.
Create a map that is shared between server sessions that links a session id to a display driver. This allows multiple client drivers to write to the same display driver and thus the same catalogue in Gaffer.
I haven't added tests yet, but does the code look good so far?
Checklist