Maya-128422 Override PrimReader

[samuelliu-adsk]

The Register functions are overloaded to provide backward compatibility. Registering a prim reader without providing ContextSupport will be treated as "fallback"

[calligrapheric]

Would it bring even more benefit to pass along the CanImport method, the prim being imported so the CanImport method can inspect the prim and see it matches possible even more restrictive conditions for the reader to be appropriate? for example, a given LightPrim having attributes specializing the import (using a specific reader) but otherwise, the user would default back to the Maya Light importer. It would add an even wider scope of possibilities for the reader to be filtered based, not only the arguments being passed, but also on the prim description they are handling.

[vlasovi]

That feature would be harder to impement and it may have a negative performance impact during the import, so I would wait with implementing it until we have a specific customer request.

[vlasovi]

Actually, I just double-checked and we call UsdMayaPrimReaderRegistry::FindOrFallback for every single prim we exprort, so implementing this feature will not be that hard as I thought. And since we call FindOrFallback on every prim (although we don't have to now), the performance impact is already there.

[samuelliu-adsk]

Hi @calligrapheric. Neil proposed a way to set priority of readers is through JobContext, so that customers can specify which reader to use for a certain Prim type. Do you think that will solve the situation you mentioned? If so I think the job context does provide more flexibility to customers.

[calligrapheric]

@samuelliu-adsk - it would not solve the issue as I am not suggesting to replace the default Maya reader but to enable the reader registry to make a better discrimination from the available readers for a prim type. The same Prim type could contain or not vendor specific attributes that would be of interest for a given reader. If the prim attributes can be inspected at the CanImport call, it would enable the reader to report it supports the specialized prim or not, otherwise falling back to the default Maya reader. That is my suggestion. I am not against the JobContext idea, but I think that using the CanImport facility is easier on users for now.

The 3ds Max prim writers are working in a similar fashion and it brings flexibility.

[samuelliu-adsk]

Hi @calligrapheric. Good point. For this PR I think we can focus on CanImport for now.

a given LightPrim having attributes specializing the import (using a specific reader) but otherwise, the user would default back to the Maya Light importer

After some thoughts, I think it's a good design but I also doubt inspecting the prim on our side will do much help...

1. What if the customer wants to override the default prim reader regardless it contains special attributes or not? In that case we will be forcing it back to default reader..
2. Shouldn't the customized prim reader be able to handle non-special attributes already? If not they can't read prims properly..

Let me know if I'm wrong! I would love to hear more suggestions from you!

[calligrapheric]

The CanImport static method only receives the import args. I propose we add the UsdPrim the PrimReader is asked to act on. To answer your points:

1. In my proposition, the PrimReader has the option of inspecting the UsdPrim. To override the default reader, it simply needs to reply it is a supported reader regardless of the UsdPrim content, as in your PR. If two plugins provide a PrimReader supporting the same prim type, then we would need Neil's JobContext for the user to select the one to keep.
2. In the actual state, the PrimReader Read method can handle all attributes (standard or vendor specific). That is not the question. But the CanImport static method has no clue on which UsdPrim (besides its type) it is replying.

[vlasovi]

This block should be deleted, not just commented out

[vlasovi]

Looks good to me. The only thing is to remove the commented out code.

[calligrapheric]

might want to remove this commented line

[calligrapheric]

The FactoryFnWrapper already handles that situation (see line 129)
maybe use something along those lines

	static void Register(boost::python::object cl, const std::string& typeName)
	{
		bool updated = false;
		FactoryFnWrapper fn = FactoryFnWrapper::Register(cl, typeName, updated);
		if (!updated)
		{
			auto type = TfType::FindByName(typeName);
			MayaUsdPrimReaderRegistry::Register(type, fn, fn, true);
		}
	}

[calligrapheric]

might also want to remove line 572

[seando-adsk]

I am not familiar with this code. I had a quick look at the changes and they look ok to me. I'll let the two other reviewers provide code review feedback.