Use DerivedMesh instead of Mesh in COLLADA GeometryExporter

GeometryExporter refactor to allow use of DerivedMesh rather than Mesh. I took the opportunity and cleaned up some of the class' interface, as well as, tried to improve consistency.

[jesterKing, 2011-05-02 22:46:43]

Patch looks fine to me to as a basis to work from in the future.

[dfelinto, 2011-05-02 23:05:24]

Some comments on syntax:
* Try to follow the file syntax (e.g. if { in oppose to if \n {).
* Also as a general rule of thumb patch should have only the essential code on
it. e.g. changes of tabs stripping in the beginning of lines should be avoided
(they led to misleading traceback of commits when using svnblame/log.

It's always recommended to use meld (linux) or svntortoise base (win) to clean
it first.

Other than that patch LGTM. Although I'm not familiar with this area of code.

http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
File blender/source/blender/collada/GeometryExporter.cpp (left):

http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
blender/source/blender/collada/GeometryExporter.cpp:30: 
better not change lines if not needed. The cleaner the patch the better (it
helps svn praise/blame/log later)

http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
File blender/source/blender/collada/GeometryExporter.cpp (right):

http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
blender/source/blender/collada/GeometryExporter.cpp:47: 
same comment as above.

http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
blender/source/blender/collada/GeometryExporter.cpp:127: 
avoid changes with no real change (e.g. stripping off tabs).

http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
blender/source/blender/collada/GeometryExporter.cpp:408: if(!smoothnormal)	//
flat
use { in the same line as if (i.e. follow file syntax)

http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
blender/source/blender/collada/GeometryExporter.cpp:413: {
same as above

[brechtvl, 2011-05-03 07:14:52]

This patch is not using DerivedMesh correctly, don't know how it can even work,
at least uv and vertex color export would probably crash.

http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
File blender/source/blender/collada/GeometryExporter.cpp (right):

http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
blender/source/blender/collada/GeometryExporter.cpp:73: DerivedMesh *dm =
mesh_create_derived_view(mScene, ob, CD_MASK_BAREMESH);
Mask should include the layers you will use, i.e. CD_MASK_MTFACE, CD_MASK_MCOL.
Also, the derivedmesh is not being released at the end.

http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
blender/source/blender/collada/GeometryExporter.cpp:89: CustomData *face_data =
static_cast<CustomData *>(dm->getFaceDataArray(dm, CD_MTFACE));
This can't work, dm->getFaceDataArray does not return CustomData*, it returns
the actual layer data if it exists. dm->faceData has it.

[official.address243, 2011-05-04 20:52:25]

I integrated that patch into:
http://codereview.appspot.com/4425084/

I try to apply your review comments. Thanks. Comment on failure in
"dm->getFaceDataArray" was a big help!

[official.address243, 2011-05-04 22:06:45]

Patch on http://codereview.appspot.com/4425084/ fixes the problem with the
wrongly casted CustomData*. But now all normals are wongly exported, all faces
are flat.

[dBugSlayer, 2011-05-05 06:04:50]

On 2011/05/02 23:05:24, dfelinto wrote:
> Some comments on syntax:
> * Try to follow the file syntax (e.g. if { in oppose to if \n {).
> * Also as a general rule of thumb patch should have only the essential code on
> it. e.g. changes of tabs stripping in the beginning of lines should be avoided
> (they led to misleading traceback of commits when using svnblame/log.
> 
> It's always recommended to use meld (linux) or svntortoise base (win) to clean
> it first.
> 
> Other than that patch LGTM. Although I'm not familiar with this area of code.
> 
>
http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
> File blender/source/blender/collada/GeometryExporter.cpp (left):
> 
>
http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
> blender/source/blender/collada/GeometryExporter.cpp:30: 
> better not change lines if not needed. The cleaner the patch the better (it
> helps svn praise/blame/log later)
> 
>
http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
> File blender/source/blender/collada/GeometryExporter.cpp (right):
> 
>
http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
> blender/source/blender/collada/GeometryExporter.cpp:47: 
> same comment as above.
> 
>
http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
> blender/source/blender/collada/GeometryExporter.cpp:127: 
> avoid changes with no real change (e.g. stripping off tabs).
> 
>
http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
> blender/source/blender/collada/GeometryExporter.cpp:408: if(!smoothnormal)	//
> flat
> use { in the same line as if (i.e. follow file syntax)
> 
>
http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
> blender/source/blender/collada/GeometryExporter.cpp:413: {
> same as above

Hi Dalai,

Thanks for the feedback.

Do you mean using meld/tortoise to clean the trailing spaces/tabs as its own
patch? What's the general approach towards this? I'm assuming there's nothing
preventing people from checking-in code with trailing tabs/spaces.

[dBugSlayer, 2011-05-05 06:37:19]

On 2011/05/03 07:14:52, brechtvl wrote:
> This patch is not using DerivedMesh correctly, don't know how it can even
work,
> at least uv and vertex color export would probably crash.
> 
>
http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
> File blender/source/blender/collada/GeometryExporter.cpp (right):
> 
>
http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
> blender/source/blender/collada/GeometryExporter.cpp:73: DerivedMesh *dm =
> mesh_create_derived_view(mScene, ob, CD_MASK_BAREMESH);
> Mask should include the layers you will use, i.e. CD_MASK_MTFACE,
CD_MASK_MCOL.
> Also, the derivedmesh is not being released at the end.
> 
>
http://codereview.appspot.com/4387052/diff/1/blender/source/blender/collada/G...
> blender/source/blender/collada/GeometryExporter.cpp:89: CustomData *face_data
=
> static_cast<CustomData *>(dm->getFaceDataArray(dm, CD_MTFACE));
> This can't work, dm->getFaceDataArray does not return CustomData*, it returns
> the actual layer data if it exists. dm->faceData has it.

Hi Bretch,

Thanks for reviewing the code.

Yes, the section that exports UV and vertex color is broken as of this patch.
I'm still learning the API and trying to figure how to handle that.

What exactly is stored on the CD_MTFACE layer? Does it contains an array of
MTFaces?

I can't recall for certain, but I think the reason why I ended up casting it to
CustomData* was because I was getting errors. Also, after reading the comments
right above getFaceDataArray definition I understood that it would return
CustomData*'s. But clearly, I was mistaken. This is what BKE_DerivedMesh.h says
about getFaceDataArray:

...
	/* return a pointer to the entire array of vert/edge/face custom data
	 * from the derived mesh (this gives a pointer to the actual data, not
	 * a copy)
	 */
...

[official.address243, 2011-05-05 09:42:11]

Hello "dBugSlayer",
I suggest we work together. It would be great if you would apply my patch:
http://codereview.appspot.com/4425084/, and continue your work with that patch.
I fixed already some of the stuff Brecht mentioned. I suggest we fix at first
the code, so it works flawless, then going after all wrong spaces, newlines,
etc..

[dBugSlayer, 2011-05-05 15:29:04]

Hi official.address243,

I already looked at some of your changes last night. I'll apply your
changes and continue working based off of that.

I'm not try to go after all trailing spaces/tabs. But I have the habit
of cleaning some of these minor things as I browse through the code.
Since these changes tends to clutter the patch, I started putting them
in a separate branch. That way I can send that as a separate patch.
That goes for typos as well.

Is the stuff that you're working on part of a GSoc2011 project?

I look forward to working with you on this.

Cheers,

Daniel

On May 5, 2011, at 2:42, "official.address243@googlemail.com"
<official.address243@googlemail.com> wrote:

> Hello "dBugSlayer",
> I suggest we work together. It would be great if you would apply my
> patch: http://codereview.appspot.com/4425084/, and continue your work
> with that patch. I fixed already some of the stuff Brecht mentioned. I
> suggest we fix at first the code, so it works flawless, then going after
> all wrong spaces, newlines, etc..
>
> http://codereview.appspot.com/4387052/

[official.address243, 2011-05-05 15:35:27]

> Is the stuff that you're working on part of a GSoc2011 project?
No, but I'm one of the poor guys who actually chose to work with the COLLADA
file format and I need it working ... NOW!
Plus: I'm actually a programmer, NO artist (and never will be), so why don't fix
the problem myself?

[dBugSlayer, 2011-05-05 15:44:34]

I'm a programmer as well. And choose to help improve Blender's COLLADA support.

I saw your comment about normals not being exporter correctly. I think
I did fix a bug on GeometryExporter::getNormals a couple weeks ago.
Although your patch may already have that change and you could be
running into a different issue.

I also have code to export the tangents. It's not compete and not
fully tested yet, but if you'd like I can share that with you later
tonight.

Daniel

On May 5, 2011, at 8:35, "official.address243@googlemail.com"
<official.address243@googlemail.com> wrote:

>> Is the stuff that you're working on part of a GSoc2011 project?
> No, but I'm one of the poor guys who actually chose to work with the
> COLLADA file format and I need it working ... NOW!
> Plus: I'm actually a programmer, NO artist (and never will be), so why
> don't fix the problem myself?
>
> http://codereview.appspot.com/4387052/

[official.address243, 2011-05-05 15:58:05]

On 2011/05/05 15:44:34, dBugSlayer wrote:
> I saw your comment about normals not being exporter correctly. I think
> I did fix a bug on GeometryExporter::getNormals a couple weeks ago.
> Although your patch may already have that change and you could be
> running into a different issue.
> 
> I also have code to export the tangents. It's not compete and not
> fully tested yet, but if you'd like I can share that with you later
> tonight.
Great to hear that. Tangets was always a thing I was missing.
I already tried to remove your changes to the normal export and used the old
one. It worked great. All should-be-smooth normals are smooth, and all
should-be-flat normals are flat.
But if you already fixed that, great. You could upload your changes so I can see
if it works now with complex models.

But the export is still messed up: There's a reference to colored vertices, but
the referenced source doesn't exist. Instead the UV Layer source is written, but
not referenced.
Have your tested your patch with colored vertices and/or a UV layer?

PS: Tonight I'm not available, but tomorrow would be great.

PPS: I think I wait until you uploaded your patch (including my earlier work,
using your earlier work), before I work any further, because it would get too
confusing merging everything. :)

[dBugSlayer, 2011-05-05 16:08:40]

I'll update my changes to be based off of what you have on Codereivew.
That way it's easier to see what changes I've made.

I haven't tested anything related to UVs or vertex colors. I'd assume
they're broken ATM.

[jesterKing, 2011-06-07 14:07:32]

On 2011/05/05 16:08:40, dBugSlayer wrote:
> I'll update my changes to be based off of what you have on Codereivew.
> That way it's easier to see what changes I've made.
> 
> I haven't tested anything related to UVs or vertex colors. I'd assume
> they're broken ATM.

Hi,

Have you made nay progress with your patch? See
http://codereview.appspot.com/4530081/ for code by official.address243 together
with changes made by pjoe for fixed normals (see the patch link).

Regards,

/Nathan

[dBugSlayer, 2011-06-08 09:17:50]

Hi Nathan,

Sorry, I have been caught up with work these past few weeks and did
not have a chance to work on it. Although, I did rebase all of my
changes along with the ones official.address243 posted and sent him a
set of patches for him to apply on his git branch. Not sure if he made
any progress.

 I'm glad to see that other people were able to improve the changes I
put on CodeReview.

I should have some time available this weekend to work on the
exporter. I'll take a look at pjoe's patch to see what else is left to
do.

Cheers,

Daniel

On Jun 7, 2011, at 7:07, "nathan.letwory@gmail.com"
<nathan.letwory@gmail.com> wrote:

> On 2011/05/05 16:08:40, dBugSlayer wrote:
>> I'll update my changes to be based off of what you have on Codereivew.
>> That way it's easier to see what changes I've made.
>
>> I haven't tested anything related to UVs or vertex colors. I'd assume
>> they're broken ATM.
>
> Hi,
>
> Have you made nay progress with your patch? See
> http://codereview.appspot.com/4530081/ for code by official.address243
> together with changes made by pjoe for fixed normals (see the patch
> link).
>
> Regards,
>
> /Nathan
>
> http://codereview.appspot.com/4387052/

[pjoe, 2011-06-08 09:29:03]

Hi,

Let me know if you need some help with the normals stuff. It wasn't that big a
change, but the patch has grown quite large. If it's easier I can help getting
the normals fix in on top of your stuff instead of trying to merge the whole
patchset.

[dBugSlayer, 2011-06-17 07:36:12]

On 2011/06/08 09:29:03, pjoe wrote:
> Hi,
> 
> Let me know if you need some help with the normals stuff. It wasn't that big a
> change, but the patch has grown quite large. If it's easier I can help getting
> the normals fix in on top of your stuff instead of trying to merge the whole
> patchset.

Hi pjoe,

After some fiddling around with your patch, I was able to apply my changes on
top of it and got Blender to compile. I haven't had much time to work on this
but I'll keep you guys posted on my progress. It's been slow, but eventually
I'll get something done.

What are we going to do about all these changes? So far, I'm working with your
patch and the changes I got from official.address243. The changes I'm making are
been done on top of them, but I'm not sure if we're merging all them in one
entry here in codereview or if we should keep them separate.

Cheers,
Daniel

[pjoe, 2011-06-17 08:09:48]

On 2011/06/17 07:36:12, dBugSlayer wrote:
> After some fiddling around with your patch, I was able to apply my changes on
> top of it and got Blender to compile. I haven't had much time to work on this
> but I'll keep you guys posted on my progress. It's been slow, but eventually
> I'll get something done.

Great to hear, can you add your updated patch here when it's ready?

> What are we going to do about all these changes? So far, I'm working with your
> patch and the changes I got from official.address243. The changes I'm making
are
> been done on top of them, but I'm not sure if we're merging all them in one
> entry here in codereview or if we should keep them separate.
> 

Been a bit concerned about this too. It's quite a large patch by now (at least
for a noob blender dev as me). If there is a good way to split it up in smaller
pieces, I think that would be best, but there are probably a lot of dependencies
that makes it difficult.

If you want to get hold of me, I normally hang around in #blendercollada on irc.

Happy coding,

 -Pelle

[jesterKing, 2011-06-17 09:46:35]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 17.6.2011 11:09, pelle.johnsen@gmail.com wrote:
> On 2011/06/17 07:36:12, dBugSlayer wrote:
>> What are we going to do about all these changes? So far, I'm working
> with your
>> patch and the changes I got from official.address243. The changes I'm
> making are
>> been done on top of them, but I'm not sure if we're merging all them
> in one
>> entry here in codereview or if we should keep them separate.
> 
> 
> Been a bit concerned about this too. It's quite a large patch by now (at
> least for a noob blender dev as me). If there is a good way to split it
> up in smaller pieces, I think that would be best, but there are probably
> a lot of dependencies that makes it difficult.

It would indeed be nice to have these as smaller patches, but as Pelle
already mentions, the dependencies for the different parts make that
very hard. I think that what could be separated pretty easily is the
"export selection" part. If you guys could give that a shot, we could
have these two separate things reviewed in a faster pace and applied too
(at least the "export selection" would be nice to have this/next week in
trunk).

/Nathan

> 
> http://codereview.appspot.com/4387052/


- -- 
Nathan Letwory
Letwory Interactive | Studio Lumikuu
http://www.letworyinteractive.com | http://www.lumikuu.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (MingW32)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJN+yJxAAoJEKtfN7KsE0TtcnkH+wcc3Oi+X4OBxMYVDhWJjEiy
HgSh9BBbae1gCLfQXIua88QjIP9lApoi6crxR7lqmSm15wT1MM6YM8wlc+vH/I2F
ER5LDTqeNS7GFBMFIdbo26QNWcWHc7zsGjFoh+ynuE57G01xeaPkr/4ojrR03i/q
1KRkUl56RsifHk/7iy61jk/TSlebLjgVnQDY20/aLhodJSzhYAbxok1QZTFw208z
G+tKJKdpBZxNHBuFGEcP3/E+alojlUwG1+EFKrB2g/vZCW1Gldd8UogqTpeS3izM
oVNth943eGfSXkqhX/4eC+B4OIWS7+Vr3rfpo3HNxhOfVDsqQW0/Lb8jo4CJM7k=
=eI/x
-----END PGP SIGNATURE-----

[official.address243, 2011-06-17 12:47:16]

On 2011/06/17 09:46:35, jesterKing wrote:
> It would indeed be nice to have these as smaller patches, but as Pelle
> already mentions, the dependencies for the different parts make that
> very hard. I think that what could be separated pretty easily is the
> "export selection" part. If you guys could give that a shot, we could
> have these two separate things reviewed in a faster pace and applied too
> (at least the "export selection" would be nice to have this/next week in
> trunk).
> 
> /Nathan

Yes, move "export selection" into the trunk, that would be nice and, I'm sure,
already very helpful for many people!
So that all patches are based on that code.
But removing it completely and later re-apply it as separate patch wouldn't make
sense.
Because of 2 reasons:
1.) It's basically the simplest part of the patch, most easily to understand. So
removing it wouldn't help much in understanding the code better.
2.) While it's simple, it is spread over many files. So if you reapply it later
it's much more work to reapply it with all the then changed code. The "export
selection" patch is more a Sisyphus work than a "brainer" (that means also an
unlikely source for errors).

So, yes, please merge it into the trunk, and leave the rest for later, I'm all
for it :)

[dBugSlayer, 2011-06-18 05:13:53]

I won't have access to a computer this weekend. Pelle, could you put
together a patch with the export selection code? Since you've fixed
the normal bug, I'd imagine you're familiar with most of the recent
changes.

Aside from the changes I've posted on CodeReview, I have partial
implementation for exporting the tangents. But we'll worry about this
once we get the "export selection" into trunk.

Thanks,

Daniel

On Jun 17, 2011, at 2:46, Nathan Letwory <nathan.letwory@gmail.com> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 17.6.2011 11:09, pelle.johnsen@gmail.com wrote:
>> On 2011/06/17 07:36:12, dBugSlayer wrote:
>>> What are we going to do about all these changes? So far, I'm working
>> with your
>>> patch and the changes I got from official.address243. The changes I'm
>> making are
>>> been done on top of them, but I'm not sure if we're merging all them
>> in one
>>> entry here in codereview or if we should keep them separate.
>>
>>
>> Been a bit concerned about this too. It's quite a large patch by now (at
>> least for a noob blender dev as me). If there is a good way to split it
>> up in smaller pieces, I think that would be best, but there are probably
>> a lot of dependencies that makes it difficult.
>
> It would indeed be nice to have these as smaller patches, but as Pelle
> already mentions, the dependencies for the different parts make that
> very hard. I think that what could be separated pretty easily is the
> "export selection" part. If you guys could give that a shot, we could
> have these two separate things reviewed in a faster pace and applied too
> (at least the "export selection" would be nice to have this/next week in
> trunk).
>
> /Nathan
>
>>
>> http://codereview.appspot.com/4387052/
>
>
> - --
> Nathan Letwory
> Letwory Interactive | Studio Lumikuu
> http://www.letworyinteractive.com | http://www.lumikuu.com
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (MingW32)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
>
> iQEcBAEBAgAGBQJN+yJxAAoJEKtfN7KsE0TtcnkH+wcc3Oi+X4OBxMYVDhWJjEiy
> HgSh9BBbae1gCLfQXIua88QjIP9lApoi6crxR7lqmSm15wT1MM6YM8wlc+vH/I2F
> ER5LDTqeNS7GFBMFIdbo26QNWcWHc7zsGjFoh+ynuE57G01xeaPkr/4ojrR03i/q
> 1KRkUl56RsifHk/7iy61jk/TSlebLjgVnQDY20/aLhodJSzhYAbxok1QZTFw208z
> G+tKJKdpBZxNHBuFGEcP3/E+alojlUwG1+EFKrB2g/vZCW1Gldd8UogqTpeS3izM
> oVNth943eGfSXkqhX/4eC+B4OIWS7+Vr3rfpo3HNxhOfVDsqQW0/Lb8jo4CJM7k=
> =eI/x
> -----END PGP SIGNATURE-----

[pjoe, 2011-06-18 06:53:41]

I have some time tomorrow, so I'll give it a shot :)

Btw. let me know if you need some help testing tangent export. I would also
suggest making tangent/binormals export an option, so in those cases where the
receiving app. e.g. generates those on its own, they won't 'bloat' the dae file.
Maybe you are already doing this :)

Happy coding,

 -Pelle

On 2011/06/18 05:13:53, dBugSlayer wrote:
> I won't have access to a computer this weekend. Pelle, could you put
> together a patch with the export selection code? Since you've fixed
> the normal bug, I'd imagine you're familiar with most of the recent
> changes.
> 
> Aside from the changes I've posted on CodeReview, I have partial
> implementation for exporting the tangents. But we'll worry about this
> once we get the "export selection" into trunk.
> 
> Thanks,
> 
> Daniel
>

[pjoe, 2011-06-19 20:15:11]

I've created http://codereview.appspot.com/4636051/, where I've tried to extract
the 'export selected' part. I haven't done extensive testing but so far it seams
to work :)