KhronosGroup
Unique ID generation bug with invalid files using duplicated ids
As I understand it, the unique ID is supposed to be unique for each object in the scene. However, here is how it's implemented in the code:
const COLLADAFW::UniqueId& Loader::getUniqueId( const COLLADABU::URI& uri, COLLADAFW::ClassId classId ) { URIUniqueIdMap::iterator it = mURIUniqueIdMap.find(uri); if ( it == mURIUniqueIdMap.end() ) { return mURIUniqueIdMap[uri] = COLLADAFW::UniqueId(classId, mLoaderUtil.getLowestObjectIdFor(classId), getFileId(uri)); } else { return it->second; } }
I.e. it's mapped just by url, ignoring the class type. So if the file has a mesh and a node named in the same way for example, they will both end up with the same unique id. Also the second object will report wrong class. Example file can be found in three.js: http://threejs.org/examples/#webgl_loader_collada_keyframe
ID is supposed to be unique in the document (not in the scene).
On Wed, Jun 22, 2016 at 3:38 AM, ivalylo [email protected] wrote:
As I understand it, the unique ID is supposed to be unique for each object in the scene. However, here is how it's implemented in the code:
const COLLADAFW::UniqueId& Loader::getUniqueId( const COLLADABU::URI& uri, COLLADAFW::ClassId classId ) { URIUniqueIdMap::iterator it = mURIUniqueIdMap.find(uri); if ( it == mURIUniqueIdMap.end() ) { return mURIUniqueIdMap[uri] = COLLADAFW::UniqueId(classId, mLoaderUtil.getLowestObjectIdFor(classId), getFileId(uri)); } else { return it->second; } }
I.e. it's mapped just by url, ignoring the class type. So if the file has a mesh and a node named in the same way for example, they will both end up with the same unique id. Also the second object will report wrong class. Example file can be found in three.js: http://threejs.org/examples/#webgl_loader_collada_keyframe
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/KhronosGroup/OpenCOLLADA/issues/446, or mute the thread https://github.com/notifications/unsubscribe/AAIsuR4ozwRiuDa3xpFr5phJflj-04jPks5qORC_gaJpZM4I7ot7 .
I'm talking about the UniqueId class, which holds 3 field - the object id, the class id and the file id. So it should work across multiple documents (and IMO, there is no problem with that). The problem is wrong implementation of getUniqueId(). I can paste my quick fix here, but the code tag seems to not work properly for me
I'm not sure to understand the issue with the code. I checked the dae loaded at http://threejs.org/examples/#webgl_loader_collada_keyframe and it is not valid. https://github.com/mrdoob/three.js/blob/master/examples/models/collada/pump/pump.dae contains duplicated IDs. For example: <geometry id="gearCover" name="gearCover"> and <node id="gearCover" name="gearCover" type="NODE">
This is not valid COLLADA and leads to the issue you mentioned.
Well, invalid or not, these are the sort of files that people work with. Let's check out who exported this invalid file, which is pretty popular:
"<authoring_tool>OpenCOLLADA2010 x64</authoring_tool>"
It's good to talk about specs, but here is a real-world case - OpenCOLLADA can't load properly it's own export due a bug. Don't know if it was fixed, the file is 6 years old, but the software should handle invalid files as gracefully as possible. Making UniqueId more general to handle such cases is something that makes sense to me. If this doesn't make sense to you, just close the issue. I work in my own branch, so it doesn't matter for me, just reporting.
How do you propose the loader to be able to automatically fix those files? Could you provide a PR ?
I think the bug title need to be changed, because we are not talking about a unique ID generation bug, but we are talking about allowing import of invalid file with duplicated ID.
am I correct ?
Well, for me - this is exactly the only issue. The code assumes that the file will not have duplicated ids, otherwise it overrides the existing id. It should give a valid id, no matter if this is allowed by the spec. That's it, I don't think OpenCOLLADA can do anything else about it, except maybe issuing a warning. There is one big side effect that I can see with such invalid naming - animations will be assigned to both objects. This is the right behavior IMO, since this is what is written in the file and all the information is provided to the user of the library. Since the objects will be from different classes, they most probably use different animation classes, and the application itself can just ignore animations which don't make sense for the particular object. It must work safely with such thing anyway. My fix is quite simple:
COLLADASaxFWLLoader.h
typedef COLLADABU::hash_map<COLLADABU::URI, COLLADAFW::UniqueId> URIUniqueIdMap;
COLLADASaxFWLLoader.cpp
const COLLADAFW::UniqueId& Loader::getUniqueId( const COLLADABU::URI& uri, COLLADAFW::ClassId classId )
{
URIUniqueIdMap::iterator it = mURIUniqueIdMap.lower_bound(uri);
while (it != mURIUniqueIdMap.end() && it->first == uri) {
if (it->second.getClassId() == classId) {
return it->second;
}
++it;
}
COLLADAFW::UniqueId uqniueId(classId, mLoaderUtil.getLowestObjectIdFor(classId), getFileId(uri));
it = mURIUniqueIdMap.insert(it, std::pair<COLLADABU::URI, COLLADAFW::UniqueId>(uri, uqniueId));
return it->second;
}
it would make our life so much easier if you could create a PR.
clone the projet, make the change in your project, create PR.
thanks!
On Thu, Jun 30, 2016 at 1:53 AM, ivalylo [email protected] wrote:
Well, for me - this is exactly the only issue. The code assumes that the file will not have duplicated ids, otherwise it overrides existing id. It should give a valid id, no matter if this is allowed by the spec. That's it, I don't think OpenCOLLADA can do anything else about it, except maybe issuing a warning. There is one big side effect that I can see with such invalid naming - animations will be assigned to both objects. This is the right behavior IMO, since this is what is written in the file and all the information is provided to the user of the library. Since the objects will be from different classes, they most probably use different animation classes, and the application itself can just ignore animations which doesn't make sense for the particular object. It must work safely with such thing anyway. My fix is quite simple:
COLLADASaxFWLLoader.h
typedef COLLADABU::hash_map<COLLADABU::URI, COLLADAFW::UniqueId> URIUniqueIdMap;
COLLADASaxFWLLoader.cpp
const COLLADAFW::UniqueId& Loader::getUniqueId( const COLLADABU::URI& uri, COLLADAFW::ClassId classId ) { URIUniqueIdMap::iterator it = mURIUniqueIdMap.lower_bound(uri); while (it != mURIUniqueIdMap.end() && it->first == uri) { if (it->second.getClassId() == classId) { return it->second; } ++it; } COLLADAFW::UniqueId uqniueId(classId, mLoaderUtil.getLowestObjectIdFor(classId), getFileId(uri)); mURIUniqueIdMap.insert(it, std::pair<COLLADABU::URI, COLLADAFW::UniqueId>(uri, uqniueId)); return uqniueId; }
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/KhronosGroup/OpenCOLLADA/issues/446#issuecomment-229601147, or mute the thread https://github.com/notifications/unsubscribe/AAIsuTlV_x1tchwnx96FVEQFTSssXIlaks5qQ4QQgaJpZM4I7ot7 .
The code assumes that the file will not have duplicated ids, otherwise it overrides the existing id. It should give a valid id, no matter if this is allowed by the spec.
I think overriding existing IDs is not good idea, it is just extra job for library, probably for nothing. Because consider two geometries both have same ID, and this ID referenced in <node> -> <instance_geometry url="#second">, so if you change second geometry's ID and if instance_geometry originally referenced second one, then rendering results will be different. So this is not solution.
I think library should issue a warning during parsing; urls/IDs could work like javascript IDs, if you try to get an object by ID then it should return FIRST-FOUND element instead of crashing lib/app...
