if resolveIds is true, then we do no want to look at <skeleton> since we
are looking at unique ids in the document (and not at Sid from <skeleton>
so we should have

if (resolveIds) -> search by Id
if (skeletonRoots.size() == 0) -> search by Sid, but we need to set the
starting point to the root of the scene
else -> search by Sid from the start of the root

so indeed there seem to be a bug in the existing code, as when resolveIds
is true, we should use resolveId() and not resolveSid()

do you agree?



On Fri, Aug 2, 2013 at 12:01 PM, Gai Clary <notifications@github.com> wrote:

> In COLLADASaxFrameworkLoader/src/COLLADASaxFWLDocumentProcessor.cpp:
>
> > @@ -317,8 +317,9 @@
> >
> >  			bool jointFound = false;
> >
> > -			if ( resolveIds )
> > +			if ( (!resolveIds) && skeletonRoots.size() == 0)
>
> First: Yes adding a comment would be good :)
> But i disagree with your proposal. Lets step through the code and start
> at:
> COLLADASaxFWLLibraryControllersLoader.cpp(line 330)
>
> StringListMap::const_iterator itSid = mJointSidsMap.find(sourceId);
> // check if the node sid array could be found
> if ( itSid != mJointSidsMap.end() )
> {
>     nodeSidsOrIds = &itSid->second;
>     isIdArray = false;
> }
>
> so i believe that *isIdArray* is set to false when *nodeSidsOrIds*contains only sids.
>
> further down (line 366):
>
> bool success = getFileLoader()->createAndWriteSkinController(
>     instanceControllerData,
>     controllerUniqueId,
>     mCurrentControllerSourceUniqueId,
>     *nodeSidsOrIds,
>     isIdArray);
>
> So here *nodeSidsOrIds* contains sids and *isIdArray* is still false.
> Now lets look at the called function itself,
> which is COLLADASaxFWLLibraryControllersLoader.cpp (line 301):
>
> bool DocumentProcessor::createAndWriteSkinController(
>     const Loader::InstanceControllerData& instanceControllerData,
>     const COLLADAFW::UniqueId& controllerDataUniqueId,
>     const COLLADAFW::UniqueId& sourceUniqueId,
>     const StringList& sidsOrIds,
>     bool resolveIds)
>
> So *sidsOrIds* contains Sids and *resolveIds* is false.
> Further down (line 320) i believe there is a bug:
>
> if ( resolveIds)
> {
>     // So here we have the part that will be called when resolveIds is true,
>     // so when the stringArray does contain ids. And this MUST be wrong
>     // because the following call can not work on Ids:
>     // const SidTreeNode* joint = resolveSid( sidOrId );
>
> Hence i believe strongly that the if and else blocks must be exchanged:
>
> if (!resolveIds)
> {
>     // Now we step in here when the sidOrId contains sids,
>     // and the code makes sense to me, BUT !!! ...
>
> We should only step into the if block when we have no skeleton roots
> defined:
>
> if ( skeletonRoots.size() == 0 && !resolveIds)
> {
>     // Now this part is only called when there is no skeleton defined AND
>     // when there are only Sids in the sidorId array.
>     ....
>
> And last look at the else block:
>
> else
> {
>     // we get the list from the <skeleton> entries
>     for ( URIList::const_iterator skeletonIt = skeletonRoots.begin();
>         skeletonIt != skeletonRoots.end();
>         ++skeletonIt)
>     {
>         // we come here only if skeletonRoots contains entries.
>         // And that is exactly what we want.
>
> —
> Reply to this email directly or view it on GitHub<https://github.com/KhronosGroup/OpenCOLLADA/pull/209/files#r5560297>
> .
>