Differential D92

Importing Vertex Colors Using OpenCollada
ClosedPublic
Actions

Authored by Saurabh Wankhade (sauraedron) on Dec 10 2013, 5:36 PM.

Details

Reviewers

Gaia Clary (gaiaclary)

Maniphest Tasks

T39967: collada import of vertex color fails

Summary

patch for importing Vertex Colors using Opencollada

Diff Detail

Event Timeline

Gaia Clary (gaiaclary) added inline comments.Dec 11 2013, 12:45 AM

source/blender/collada/MeshImporter.cpp
539	Below this line something odd seems to happen with the whitespaces. In order to get an easier comparison, would it be possible to fix this ? (Probably just replace whitespaces by tabs ?)

Saurabh Wankhade (sauraedron) updated this revision to Unknown Object (????).Dec 11 2013, 5:41 AM

I did not test the patch, i only looked at the code changes and i added a few comments.
Also, did you test if Blender can export vcols and later import vcols without loosing/changing information ?

source/blender/collada/MeshImporter.cpp
201–203	If the mVData->getType() can be converted into a meaningful string, then wouldn't it be more user friendly to write: case COLLADAFW::MeshVertexData::DATA_TYPE_UNKNOWN: fprintf(stderr, "MeshImporter.getvcol(): unknown data type\n"); default: fprintf(stderr, "MeshImporter.getvcol(): unsupported data type %s\n", mVData->getType()); If there is no good way to create a meaningful datatype string, then imho you could remove line 201
447–448	From where do we know that the mesh has vertex colors ? Isn't this unnecessary to do when no vcols are available ?
592–593	What happens if the mesh has no uvs or no vcols ?
617	not needed imho (see below)
624	Could this be moved just before line 691 ?
689	if (has_vcolor(mp)) ...
source/blender/collada/MeshImporter.h
76–77	You can setup a MVDataWrapper either with uvs or with colors. later you must KNOW what data is in the wrapper in order to choose the correct get function. Wouldn't it be clearer to create 2 classes here ? One for the UVs one for the colors ?

You can setup a MVDataWrapper either with uvs or with colors. later you must KNOW what data is in the wrapper in order to choose the correct get function. Wouldn't it be clearer to create 2 classes here ? One for the UVs one for the colors ?

IMHO i don't think there's a need for defining another class

What happens if the mesh has no uvs or no vcols ?

I dont know about uvs, but now CD_MLOOPCOL data layer would not be created.

If the mVData->getType() can be converted into a meaningful string, then wouldn't it be more user friendly to write:

removed the mentioned line

Could this be moved just before line 691 ?

moved.

Also, did you test if Blender can export vcols and later import vcols without loosing/changing information ?

Yes i tested and it seems to be working. http://www.pasteall.org/pic/show.php?id=63795

From where do we know that the mesh has vertex colors ? Isn't this unnecessary to do when no vcols are available ?

we come to know if there are colors hasColorIndices()

vertex.blend531 KBDownload

vertexquad.dae219 KBDownload

vertextri.dae264 KBDownload

vertexquad.dae when triangulated is unchecked

vertextri.dae when triangulated is checked.

Created seperate class VCOLDatawrapper

I have not tested, but looks ok for me.

This task needs approval or request for changing from jesterking ;)

Gaia, you are a Collada module owner, therefore can change this on your own authority.

My work machine broke down begin this week. I havent yet time to set up dev env on the spare laptop. Better if Gaia checks and accepts instead of waiting for me.

I have applied this patch with a few minor changes to the Collada importer.
Thanks for providing this solution.

Saurabh Wankhade (sauraedron) closed this revision.Jul 24 2014, 6:47 PM