Sets #4

donboie · 2018-03-01T00:09:59Z

Hey John,

Here is the first commit of my set changes where I just read & write sets directly to the IO as you suggested in the the other PR.

Cheers

Don

donboie

So this is the first step and I hope not too contencious. I'll work on fixing what I percive to be problems & propose how I shold continue.

donboie · 2018-03-01T00:13:10Z

src/IECoreScene/SceneCache.cpp

+			{
+				ReaderImplementation *nc = const_cast<ReaderImplementation *>(this);
+				ConstPathMatcherDataPtr childSets = nc->child( c, SceneInterface::NullIfMissing )->readSet( name );
+				pathMatcherData->writable().addPaths(childSets->readable(), { c });


This will add a level to PathMatcher data after we leave a recursion level which I think can be avoided.

I'm not sure I understand. I think it's probably worth not using PathMatcherData at all during the recursion, as we're just making a wrapper that we throw away at each step. Instead we could return PathMatcher directly at each recursion and just wrap it in the outer layer. Perhaps that what you were getting at?

Instead we could return PathMatcher directly at each recursion and just wrap it in the outer layer. Perhaps that what you were getting at?

I don't think so. I was going to recurse to find all the PathMatchersData and return a list of [ ( prefix , PathMatcherData ), ... ] which I could perform PathMatcher.addPaths on the number of defined sets rather than depth I have to recurse to find a set.

Or perahps I return nothing and pass the PathMatcher in the recursion and just add to it when I find a local set.

Just wanted to mention I'll try to replace PathMatcherData with PathMatcher in anycase.

Or perahps I return nothing and pass the PathMatcher in the recursion and just add to it when I find a local set.

Sounds fine.

I don't think so.

Obviously I haven't tested, but I would expect my suggestion to have comparable performance to the one you suggest above. In case it isn't obvious what I'm suggesting, I used the same approach recently in the SceneGadget.

donboie · 2018-03-01T00:14:09Z

src/IECoreScene/SceneCache.cpp

+
+			for( const auto &c : children )
+			{
+				ReaderImplementation *nc = const_cast<ReaderImplementation *>(this);


This is a variation of the const problem I had with the SceneInterface getRoot.

Ah, so this is because ReaderImplementation::child() is non-const? What happens if we make it const?

donboie · 2018-03-01T00:16:23Z

src/IECoreScene/SceneCache.cpp

+				}
+			}
+
+			std::set<SceneInterface::Name> unique(setNames.begin(), setNames.end());


Perhaps we should add a typedef setSceneInterface::Name NameSet to SceneInterface? What do you think to updating the setNames return to NameSet?

My guess is that using std::set to make things unique is going to be a fair bit slower than a combo of std::sort and std::unique done in-place on the vector itself. I can see the logic in using a datatype that enforces uniqueness for the return value, but I suspect this is of less practical use to the caller than performance is.

Referring to code like this:

NameList children; childNames( children );

How do you feel about updating functions which return containers as reference parameters to functions which directly return them?

I think I'd like that, but I'd also leave it out of the sets work. Let's just focus on the sets for now. Maybe there's even an argument for making setNames() be pass-by-reference and then updating everything all at once later?

And I guess pass-by-reference is still better if I happen to have a suitable container already, for instance if I had a list of locations to iterate over, and at each step needed to call setNames(). I could just make a container outside the loop and then reuse it for each step.

johnhaddon

Thanks Don,

This is great - feels much better starting simply with "the bits we'll keep in the future" than diving straight in to the really complex tags<->sets interaction. I was overwhelmed by the complexity of the earlier stab, although the fact that I've now had a proper night's sleep is probably helping too.

I suspect we're going to want to do something similar to what we did for tags, so at each location we store a "descendant set names" record which we use to limit the traversal. But starting with the basics works great for me.

Once you've taken care of the few details we've highlighted, what do you see as the next step? I think it'd be great to implement sets for USD, hopefully as another uncontentious step, and to validate the sets API against another implementation before we get in too deep.

Cheers...
John

johnhaddon · 2018-03-01T17:13:17Z

src/IECoreScene/SceneCache.cpp

+			if( setsIO )
+			{
+				IndexedIO::EntryIDList localSetNames;
+				setsIO->entryIds(localSetNames, IndexedIO::Directory);


Can we read straight into setNames here, rather than make a temporary, fill it, and copy?

johnhaddon · 2018-03-01T17:21:01Z

src/IECoreScene/SceneCache.cpp

+				}
+			}
+
+			std::set<SceneInterface::Name> unique(setNames.begin(), setNames.end());


My guess is that using std::set to make things unique is going to be a fair bit slower than a combo of std::sort and std::unique done in-place on the vector itself. I can see the logic in using a datatype that enforces uniqueness for the return value, but I suspect this is of less practical use to the caller than performance is.

johnhaddon · 2018-03-01T17:27:02Z

src/IECoreScene/SceneCache.cpp

+
+			for( const auto &c : children )
+			{
+				ReaderImplementation *nc = const_cast<ReaderImplementation *>(this);


Ah, so this is because ReaderImplementation::child() is non-const? What happens if we make it const?

johnhaddon · 2018-03-01T17:29:31Z

src/IECoreScene/SceneCache.cpp

+			{
+				ReaderImplementation *nc = const_cast<ReaderImplementation *>(this);
+				ConstPathMatcherDataPtr childSets = nc->child( c, SceneInterface::NullIfMissing )->readSet( name );
+				pathMatcherData->writable().addPaths(childSets->readable(), { c });


I'm not sure I understand. I think it's probably worth not using PathMatcherData at all during the recursion, as we're just making a wrapper that we throw away at each step. Instead we could return PathMatcher directly at each recursion and just wrap it in the outer layer. Perhaps that what you were getting at?

johnhaddon · 2018-03-01T17:33:31Z

src/IECoreScene/SceneCache.cpp

+		{
+			IndexedIOPtr setsIO = m_indexedIO->subdirectory( setsEntry, IndexedIO::CreateIfMissing );
+
+			const Object *o = set;


This is annoying, isn't it? I think the problem is that the derived class save( SaveContext *context ) const protected method masks the base class save( IndexedIOPtr ioInterface, const IndexedIO::EntryID &name ) public method. All my fault, and I've been meaning to fix it.

For what it's worth I tend to use set->Object::save() to do it in one line and hopefully make it a bit more obvious why the jiggery pokery is needed.

donboie · 2018-03-01T19:48:26Z

I've address everything you've pointed out in 8415872 but I'm bothered by the ReaderImplementation::child(...) const function requiring a const cast to construct another ReaderImplementation but pointing to himself here:

cortex/src/IECoreScene/SceneCache.cpp

Line 627 in 8415872

    
           return new ReaderImplementation( childIO, const_cast<ReaderImplementation*> ( this ) );

I suspect we're going to want to do something similar to what we did for tags, so at each location we store a "descendant set names" record which we use to limit the traversal. But starting with the basics works great for me.

I have a implementation of this which I'll cherry pick & clean up.

Once you've taken care of the few details we've highlighted, what do you see as the next step? I think it'd be great to implement sets for USD, hopefully as another uncontentious step, and to validate the sets API against another implementation before we get in too deep.

I'm happy to do this as it should be a simple case.

donboie · 2018-03-01T23:00:21Z

I've added USD reading and writing of sets in a813f23. USD seems to have a limitation in that you can't add the location where the set is being writen to to the set. I suppose we should not write these in SceneCache also and emit the same warning as I've done here.

So I can either continue with futher file format support in Alembic or tackle linked scenes, what do you think?

Also I've found what looks like a bug in AddCollection in USD on a set named something like 'a:b(c)' which we have at Image Engine from Jabuka, perhaps I'll offer up a PR.

I assume this work should stay in my branch for some time or can they go into master peicemeal?

johnhaddon

I've added USD reading and writing of sets in a813f23.

Nice one.

USD seems to have a limitation in that you can't add the location where the set is being writen to to the set. I suppose we should not write these in SceneCache also and emit the same warning as I've done here.

That doesn't seem a particularly unreasonable restriction, but on the other hand it seems a bit unnecessary outside of USD, and it is possible to create a set with / in it in Gaffer. How about we just stick with the warning for USD only for now, and see if anything becomes a problem in practice?

johnhaddon · 2018-03-02T17:17:39Z

contrib/IECoreUSD/src/IECoreUSD/USDScene.h

 		void readTags( NameList &tags, int filter ) const override;
 		void writeTags( const NameList &tags ) override;
+
+		NameList setNames() const;


We'll need to add override here to keep Clang happy.

I offer 30e9a8f to gods of CLANG!

johnhaddon · 2018-03-02T17:31:46Z

Argh! GitHub sucked my review away from me before I was done. I'll copy my half-finished comment here and finish it all in one place...

I've added USD reading and writing of sets in a813f23.

Nice one.

USD seems to have a limitation in that you can't add the location where the set is being writen to to the set. I suppose we should not write these in SceneCache also and emit the same warning as I've done here.

That doesn't seem a particularly unreasonable restriction, but on the other hand it seems a bit unnecessary outside of USD, and it is possible to create a set with / in it in Gaffer. How about we just stick with the warning for USD only for now, and see if anything becomes a problem in practice?

So I can either continue with futher file format support in Alembic or tackle linked scenes, what do you think?

Whatever's best for you...

Also I've found what looks like a bug in AddCollection in USD on a set named something like 'a:b(c)' which we have at Image Engine from Jabuka, perhaps I'll offer up a PR.

Definitely sounds worth it...

I assume this work should stay in my branch for some time or can they go into master peicemeal?

Yes, I think so. I think the point to merge would be the point at which we have enough for Gaffer to stop talking tags and use sets exclusively.

Bunch of random thoughts :

Do you think PathMatcherData is the right type to use in the readSet() and writeSet() methods? Looking at some of the tests and implementation, it looks like we're often just wrapping with PathMatcherData to satisfy the API. PathMatcher itself is essentially just a smart pointer to the root node, so is perfectly fine as a lightweight way of copying things around.
I expect we'll want to speed up the USD set recursion by traversing prims directly rather than wrapping in SceneInterfaces at each stage. And it seems like a threaded traversal would be good at some point too. Do you see any problems with either? We don't need to do it now, but I wondered if you have any insights from what you've learned so far?
I should have thought of this before, but we're going to need a way of getting a hash for a set as well, aren't we? For Gaffer? Or do you think we can't do much better than just hash the filename and set name anyway?

Cheers...
John

donboie · 2018-03-02T18:21:01Z

So I can either continue with futher file format support in Alembic or tackle linked scenes, what do you think?

Whatever's best for you...

I'll probably work on the concrete Alembic implementation first followed by a sketch out my ideas for LinkedScenes. Regarding progressing with this work here, are you happy for this PR to just be appended to?

Do you think PathMatcherData is the right type to use in the readSet() and writeSet() methods? Looking at some of the tests and implementation, it looks like we're often just wrapping with PathMatcherData to satisfy the API. PathMatcher itself is essentially just a smart pointer to the root node, so is perfectly fine as a lightweight way of copying things around.

I'm leaning towards chaning the to PathMatcher, but what if the PathMatcher implemention changes to make copying them more expensive?

I expect we'll want to speed up the USD set recursion by traversing prims directly rather than wrapping in SceneInterfaces at each stage. And it seems like a threaded traversal would be good at some point too. Do you see any problems with either? We don't need to do it now, but I wondered if you have any insights from what you've learned so far?

Agreed. I think I read something that implied that the methods we use in reading GetCollection is thread safe where as the AddCollection variant isn't. Typical! I can't find the comment / doc page now.

I should have thought of this before, but we're going to need a way of getting a hash for a set as well, aren't we? For Gaffer? Or do you think we can't do much better than just hash the filename and set name anyway?

Would it need to be a hash of file name, set name and location?

johnhaddon · 2018-03-02T18:27:38Z

Regarding progressing with this work here, are you happy for this PR to just be appended to?

For sure.

but what if the PathMatcher implemention changes to make copying them more expensive?

I don't think it can - we're totally reliant on those performance characteristics elsewhere...

I think I read something that implied that the methods we use in reading GetCollection is thread safe where as the AddCollection variant isn't.

There's also that thread on the mailing list where Spiff was telling us about a new API for finding collections....

https://groups.google.com/forum/#!topic/usd-interest/O-cKw3yr-YY

UsdPrim::GetAppliedSchemas? Not sure if that's any improvement over what you're already doing though...

Would it need to be a hash of file name, set name and location?

Yes, of course. I was thinking of Gaffer where we only read sets from the root. I think it is worth having an official way of getting the set hash out of the SceneInterface, even if it is a noddy "filename+location+setName" hash, because it at least keeps the door open for better implementations in the future...

donboie · 2018-03-02T18:39:00Z

There's also that thread on the mailing list where Spiff was telling us about a new API for finding collections....
https://groups.google.com/forum/#!topic/usd-interest/O-cKw3yr-YY
UsdPrim::GetAppliedSchemas? Not sure if that's any improvement over what you're already doing though...

Ah. I'll add a todo to the USD implementation.

I don't think it can - we're totally reliant on those performance characteristics elsewhere...

I do it now then to make the LinkedScene & Alembic implementations simpler.

I think it is worth having an official way of getting the set hash out of the SceneInterface

So the API change would be more than creating a new member in the enum HashType as we need to specify the set name. Unless it's reasonable to has all sets at a location? If not we probably need:

void setHash( double time, const Name& setName, IECore::MurmurHash &h ) const; ?

I'm reminded of a previous discussions on changing the splitting up the hashing functions on SceneInterface, perhaps I should do this on master now before adding this setHash function?

johnhaddon · 2018-03-02T19:16:59Z

void setHash( double time, const Name& setName, IECore::MurmurHash &h ) const;

Having a dedicated method sounds good. We shouldn't need time though, right?

I'm reminded of a previous discussions on changing the splitting up the hashing functions on SceneInterface, perhaps I should do this on master now before adding this setHash function?

Yeah, splitting up the hash method would be good. I'm tempted to say just add the setHash() and leave the others as a todo for now, just to stay 100% focussed on sets...

donboie · 2018-03-02T20:21:24Z

void setHash( double time, const Name& setName, IECore::MurmurHash &h ) const;

Having a dedicated method sounds good. We shouldn't need time though, right?

Of course sets can't be time varying can they:

void setHash( const Name& setName, IECore::MurmurHash &h ) const

donboie · 2018-03-08T00:29:07Z

Hi John,

I've pushed this on with a few commits and wanted to have quick discussion before I attempt to progress futher. In summary I've added a noddy hashSet function, AlembicScene implementation, childSetNames to exit early from recursion and finally a sketch of the LinkedScene implementation.

Regarding the LinkedScene implementation, the crux of the issue is to quickly query sets in the scenes which are linked to. In 40a7a47 there is a function called LinkedScene::linkLocations which gets all the link locations as a pathmatcher and I was thinking this can be stored in the LinkedScene cache on save and we can rely on this being quick for the setNames & readSet implementations. What do you think?

There a few other places I'm going to fix up so I can flip the SceneInterface set functions to pure virtual:

IECoreHoudini::LiveScene
IECoreMayaScene::LiveScene
VDBScene
IERendering::ReaderScene

Have I missed any?

Cheers
Don

johnhaddon

Looks good!

Regarding the LinkedScene implementation, the crux of the issue is to quickly query sets in the scenes which are linked to. In 40a7a47 there is a function called LinkedScene::linkLocations which gets all the link locations as a pathmatcher and I was thinking this can be stored in the LinkedScene cache on save and we can rely on this being quick for the setNames & readSet implementations. What do you think?

I'm no expert when it comes to LinkedScenes, but that sounds entirely reasonable to me.

There a few other places I'm going to fix up so I can flip the SceneInterface set functions to pure virtual...Have I missed any...

Caribou's TrackedLiveScene maybe?

Cheers...
John

johnhaddon · 2018-03-08T10:26:10Z

src/IECoreScene/LinkedScene.cpp

 }

+/// serialise this into the linked scene cache so it can just be loaded directly without having to traverse the entire scene
+IECore::PathMatcherDataPtr LinkedScene::linkLocations() const


No need to return PathMatcherData here - we can rely on PathMatcher's lazy-copy-on-write and just return a PathMatcher directly. I think we planned to do this for readSet() and writeSet() too?

I think we planned to do this for readSet() and writeSet() too?

Oh yeah - addressed this in my latest changes.

donboie · 2018-03-08T22:52:30Z

src/IECoreScene/SceneCache.cpp

+			NameList childSets = readChildSets();
+
+			// todo need to replace this linear scan with something with faster query performance
+			if( std::find( childSets.begin(), childSets.end(), name ) != childSets.end() )


I'm not happy about this linear scan to work out if we should recurse. Perhaps another approach would be to store a PathMatcher of locations with sets?

donboie · 2018-03-08T22:55:46Z

src/IECoreVDB/VDBScene.cpp

+
+		void hashSet( const Name &setName, IECore::MurmurHash &h ) const override
+		{
+			h.append( fileName() );


Really the hash doesn't depend on anything here and I should not append anything to the hash but is that safe to do?

Ah, I should have thought of this earlier. All other hashes (Object, ComputeNode, SceneInterface::hash()) are first initialised by appending the TypeId of the thing in question, to avoid collisions between hashes generated by distinct types. So, in this case SceneInterface::hashSet() should remain pure virtual, but should have an implementation that just does h.append( typeId() ). Then all derived classes should first call the base class and then append anything they need.

Once that's done we could omit the fileName here because the inclusion of the TypeId will keep the hash distinct from any other SceneInterface that doesn't implement sets.

Does that make sense?

Great. Thanks for the explanation. Just one question...

So, in this case SceneInterface::hashSet() should remain pure virtual, but should have an implementation that just does h.append( typeId() )

I'm a little confused if the SceneInterface::hashSet() remains pure virtual then where should the h.append( typeId() ) go?

Maybe pure virtual isn't quite the right terminology...this is what I meant...what would you call that?

SceneInterface { virtual void hashSet( name, h ) = 0; // Force derived classes to provide an override } SceneInterface::hashSet( name, h ) // But still provide a base class implementation they can call { h.append( typeId() ); } VDBScene::hashSet( name, h ) { SceneInterface::hashSet( name, h ); // Call base class from derived class }

what would you call that?

pure virtual and I'm an idiot.

class Foo { virtual void test() = 0; // I've always assumed this means derived class must implement & there can't be implementation in Foo !!! };

Thanks for spelling it out.

readSet, writeSet & setNames functions added.

+ This will be used by the LinkedScene to write link locations on the root.

+ Kept the recurse for links code as it's probably going to be useful for upgrading old scenes.

andrewkaufman

Thanks @donboie, looks great so far. I guess other than fine tuning the details, the main remaining question is what to do about non-gaffer workflows that involve tags. I think these are the options we've discussed:

(a) Write both sets and tags for a while
- Pros
  - Less immediate work
  - We don't bottleneck our progress on tracking down and updating those workflows
  - Gaffer can start reading sets only, because that pulls in the tags if they're in the file
- Cons
  - Most/all production scenes will continue to contain only tags
    - because they're made from DCCs, not from Gaffer
(b) Update the non-gaffer workflows to use the sets api only
- Pros
  - Moves us to where we want to be
  - Makes terminology consistent between DCCs / Gaffer Power Users / Developers
- Cons
  - Workflows are likely scattered in projects you're not too familiar with
    - and that need to be dual compatible with Cortex 9/10 for a while longer
(c) Re-implement readTag to call readSet
- Pros
  - Files written with sets (Gaffer SceneWriter) will work with the non-gaffer workflows
- Cons
  - We don't want/need this code in the long run
  - Is it really much less effort than updating the other workflows?

I know I've been the main criticizer of (b), but I'm starting to wonder if I'm being overly cautious. We certainly need updates in Jabuka, and likely in RigToolbox/AnimToolbox/CfxToolbox, all of which need to be dual compatible. But are there really any others? Maybe a few Houdini OTLs. So maybe we can ask Ivan, Stefan, and Lucien to help porting those workflows over to sets, and we'd get there fairly quickly...

andrewkaufman · 2018-03-13T18:57:54Z

include/IECoreScene/SceneInterface.h


 #include "IECore/Export.h"
 #include "IECore/Object.h"
+#include "IECore/PathMatcherData.h"


just need PathMatcher.h now

andrewkaufman · 2018-03-13T18:58:38Z

include/IECoreScene/SceneInterface.h

+		/// Reads the named set. All paths returned are relative to the current location.
+		virtual IECore::PathMatcher readSet( const Name &name ) const = 0;
+		/// Writes a set at the current location. All paths are specified relative to the current location.
+		virtual void writeSet( const Name &name, IECore::PathMatcher set ) = 0;


i expected this to take a const PathMatcher &

@johnhaddon are you in agreement with @andrewkaufman on this?

andrewkaufman · 2018-03-13T19:00:55Z

src/IECoreScene/SceneInterface.cpp


+void SceneInterface::hashSet( const Name& setName, IECore::MurmurHash &h ) const
+{
+	h.append( typeId() );


we should probably hash the setName here

Just wanted to jump in quickly and say I don't think so. Two reasons - an implementation might not support sets in which case all sets have the same hash, or might be smart enough to know that two sets are identical, in which case the base class just made it impossible to report that via the hash...

andrewkaufman · 2018-03-13T19:29:52Z

src/IECoreScene/SceneCache.cpp

+
+	names.insert( names.end(), setNames.begin(), setNames.end() );
+
+	// ensure our set names are unique


In the case we don't have tags, the ReaderImplementation already sorted and unique-ified for us, right? It'd be nice to avoid the extra work if we don't need it.

andrewkaufman · 2018-03-13T19:42:15Z

contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp

+
+	h.append( m_root->fileName() );
+	h.append( &path[0], path.size() );
+	h.append( name );


feels like SceneInterface::hashSet should take care of name for us

andrewkaufman · 2018-03-13T19:45:22Z

src/IECoreVDB/VDBScene.cpp

+			return NameList();
+		}
+
+		IECore::PathMatcher readSet( const Name &name ) const override


I think its fine to not support this for now, but just wondering, didn't they introduce some concept of grouping? Would that map to sets?

As we have a single location in our VDBScene we'd have to split the VDBGrids based on point groups for sets to work. I suspect filtering on groups would be perferable.

Info on VDB PointGroups

andrewkaufman · 2018-03-13T19:45:53Z

src/IECoreMaya/LiveScene.cpp

+
+IECore::PathMatcher LiveScene::readSet( const Name &name ) const
+{
+	throw Exception( "IECoreHoudini::LiveScene::readSet not supported" );


this is maya, not houdini

also, seems like we can and should support this, the same we we do for tags:

actual maya sets

custom ieSet_whatever maya attrs

registration hooks for external plugins to monkey with the sets (asset managers)

Maybe just a todo for now so we remember to come back to it before moving out of alpha? Although, we need this before Caribou MainSceneInput can work, once Gaffer stops interpreting tags as sets, right?

I was leaving this incase the general discussion on tags vs sets might of changed what was required here.

andrewkaufman · 2018-03-13T19:48:43Z

src/IECoreHoudini/LiveScene.cpp

+
+IECore::PathMatcher LiveScene::readSet( const Name &name ) const
+{
+	throw Exception( "IECoreHoudini::LiveScene::readSet not supported" );


same comment about remembering to come back to this.

donboie · 2018-03-14T22:26:00Z

I've encountered a problem with the Set API when we copy from one scene to another. Users of the API have no simple way of knowing if a set is defined on a particular location.

How about an additional parameter to define if we include children in our set name or membership queries?

virtual NameList setNames(bool recurse = true ) const = 0;
virtual IECore::PathMatcher readSet( const Name &name, bool recurse = true ) const = 0;

Without the above we can only copy the set at the root where it might not have been originally defined or recurse down the scene looking for where sets are defined by calling setNames on every location.

johnhaddon · 2018-03-15T10:02:05Z

How about an additional parameter to define if we include children in our set name or membership queries?

Sounds good. I don't think recurse is the right name though - that's about the implementation, not the query (and I'm hoping we don't need to actually recurse at all for SceneCache::setNames()). Maybe includeDescendantSets?

This does raise the issue that a SceneReader->SceneWriter round trip in Gaffer will have moved all the sets to the root though. Are we concerned about that?

donboie · 2018-03-15T16:40:26Z

Sounds good. I don't think recurse is the right name though - that's about the implementation, not the query (and I'm hoping we don't need to actually recurse at all for SceneCache::setNames()). Maybe includeDescendantSets?

Good point, agreed.

This does raise the issue that a SceneReader->SceneWriter round trip in Gaffer will have moved all the sets to the root though. Are we concerned about that?

I can't think of a problem with this but I'm concerned something might pop up. @andrewkaufman ?

andrewkaufman · 2018-03-15T16:58:07Z

This does raise the issue that a SceneReader->SceneWriter round trip in Gaffer will have moved all the sets to the root though. Are we concerned about that?

I can't think of a problem with this but I'm concerned something might pop up. @andrewkaufman ?

I think if we're writing Tags still using that sneaky env var, we won't notice any issues until the internal Maya workflows are updated to write Sets. When we get about to doing that, I anticipate we'll usually be writing those Sets at the root anyways, unless I'm forgetting some essential workflow...

I guess the big exception to that is for LinkedScenes, but a SceneReader->SceneWriter round trip totally obliterates the links anyways, so obliterating the child Sets doesn't seem any more problematic.

Of topic, but should/could the SceneWriter maintain links somehow? Perhaps Capsules could carry along a fileName and the SceneWriter could use that to re-create links somehow? Maybe that's all nonsense... lets discuss somewhere else rather than further mudding this discussion...

donboie commented Mar 1, 2018

View reviewed changes

johnhaddon reviewed Mar 1, 2018

View reviewed changes

donboie force-pushed the sets branch from 81987cd to b755640 Compare March 1, 2018 19:29

johnhaddon reviewed Mar 2, 2018

View reviewed changes

donboie force-pushed the sets branch from a813f23 to 30e9a8f Compare March 2, 2018 18:18

donboie force-pushed the sets branch 5 times, most recently from 46c3266 to 2ef0639 Compare March 7, 2018 23:46

donboie force-pushed the sets branch from 2ef0639 to b63cf34 Compare March 8, 2018 01:39

johnhaddon reviewed Mar 8, 2018

View reviewed changes

donboie force-pushed the sets branch from b63cf34 to 78f2e2a Compare March 8, 2018 22:00

donboie commented Mar 8, 2018

View reviewed changes

donboie force-pushed the sets branch from e7c07c2 to f1afd3e Compare March 9, 2018 02:09

donboie added 6 commits March 9, 2018 11:06

IECoreScene : SceneInterface - added Set interface.

24ca455

readSet, writeSet & setNames functions added.

IECoreScene : SceneCache - set implementation.

e6774c2

IECoreScene : SceneCache - reduce recursion in set functions.

da3f145

IECoreScene : SceneCache - PathMatcherData -> PathMatcher.

adb3de9

IECoreScene : SceneCache - support full path scene query on writing.

a5b8631

+ This will be used by the LinkedScene to write link locations on the root.

IECoreScene : SceneCache - convert tags to sets in readSet & setNames

ae70a74

donboie added 12 commits March 9, 2018 11:17

IECoreScene : SceneCache - Call base hashSet.

5dc2cf0

IECoreScene : LinkedScene - set implementation.

74d95e2

IECoreScene : LinkedScene - favour PathMatcher over PathMatcherDataPtr.

6687324

IECoreScene : LinkedScene - Store link locations in root PathMatcher.

8200df6

+ Kept the recurse for links code as it's probably going to be useful for upgrading old scenes.

IECoreScene : LinkedScene - PathMatcherData -> PathMatcher.

0249f6c

IECoreScene : LinkedScene - verify tags can be read as sets.

ca6d887

IECoreScene : LinkedScene - Call base hashSet.

8482d52

IECoreUSD : USDScene - set implementation.

020039f

IECoreAlembic : AlembicScene - set implementation.

471e1e9

IECoreVDB : VDBScene - set implementation.

af540fc

IECoreMaya : LiveScene - set implementation.

913585f

IECoreHoudini : LiveScene - set implementation.

0fede5a

donboie force-pushed the sets branch from f1afd3e to 0fede5a Compare March 9, 2018 19:23

andrewkaufman reviewed Mar 13, 2018

View reviewed changes


		names.insert( names.end(), setNames.begin(), setNames.end() );

		// ensure our set names are unique

Sets #4

Are you sure you want to change the base?

Sets #4

Uh oh!

Conversation

donboie commented Mar 1, 2018

Uh oh!

donboie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donboie Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donboie Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnhaddon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donboie commented Mar 1, 2018

Uh oh!

donboie commented Mar 1, 2018

Uh oh!

johnhaddon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnhaddon commented Mar 2, 2018

Uh oh!

donboie commented Mar 2, 2018

Uh oh!

johnhaddon commented Mar 2, 2018

Uh oh!

donboie commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnhaddon commented Mar 2, 2018 • edited by donboie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

donboie commented Mar 2, 2018

Uh oh!

donboie commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnhaddon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donboie Mar 1, 2018 •

edited

Loading

donboie Mar 1, 2018 •

edited

Loading

donboie commented Mar 2, 2018 •

edited

Loading

johnhaddon commented Mar 2, 2018 •

edited by donboie

Loading

donboie commented Mar 8, 2018 •

edited

Loading