Update DisplayDriverServer #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
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
| @@ -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.
There was a problem hiding this comment.
Thanks for removing - I don't think that was a strong enough reason to expose the internals.
ivanimanishi
force-pushed
the
linasb_allowServerToMergeDrivers
branch
from
419c382 to
f0a8f9f
Compare
| struct MergeDriverInfo | ||
| { | ||
| DisplayDriverPtr mergeDriver = nullptr; | ||
| int mergeCount = 0; | ||
| }; | ||
|
|
||
| using MergeMap = std::map<int, MergeDriverInfo>; |
There was a problem hiding this comment.
Is there any reason these can't be private as well, now that m_mergeMap is?
There was a problem hiding this comment.
Since I'll be moving m_mergeMap out of the header I'll also move MergeDriverInfo out of the header.
| @@ -104,6 +111,8 @@ class IECOREIMAGE_API DisplayDriverServer : public IECore::RunTimeTyped | |||
| class PrivateData; | |||
| IE_CORE_DECLAREPTR( PrivateData ); | |||
| PrivateDataPtr m_data; | |||
|
|
|||
| MergeMap m_mergeMap; | |||
There was a problem hiding this comment.
Putting this here changes sizeof( DisplayDriverServer ) and breaks ABI - we can't do that without bumping major version. I think you can just move the MergeMap into the PrivateData struct instead. The purpose of PrivateData is to hide the member data so you can add/remove without breaking ABI.
| @@ -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.
Thanks for removing - I don't think that was a strong enough reason to expose the internals.
|
Thanks @LinasBeres, LGTM! Could you and/or @ivanimanishi squash it into a single commit and update the Changes file when merging please? |
LinasBeres
changed the title
Adds option to server to allow a 'merge' driver that shares a display driver to write to.
ivanimanishi
force-pushed
the
linasb_allowServerToMergeDrivers
branch
from
0da7e48 to
ee815ab
Compare
|
@ivanimanishi should this commit be in the PR? |
That's how we resolve the merge conflicts. I guess I could have squash-merged though, to get rid of it in the end. |
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