3DSMax fixes + support for 2012 #114

Merged
merged 14 commits into from Oct 3, 2012

Conversation

Projects
None yet
4 participants
Contributor

crazyjul commented Sep 10, 2012

Only the 2008 project is up to date.

This pull request contains lots of stuff missing or wrong in max exporter.

It also add support for tangents and binormals.

It also includes supports for max light extra data.

Sorry for the high amount of features here, but it's about a year of fixes and dev that we needed. And it's hard to cut them into smaller pieces.

If you want me to remove any changelist, I'll rebase this pull request

Contributor

fabrobinet commented Sep 10, 2012

Thanks for sharing all this Julien !
This is much much appreciated.

It's lots of changes, so I'll have to begin with a few questions :).

Is this "part 1" of fixes as it does not seem to contain the MAX 2012 fixes ?
Will the MAX 2012 fixes means that the build will also "work" with 2011 ?

Did you introduce new exported extras (I mean not already defined in the collada registry) ? If yes, can you send me an email with a sample DAE file: fabricerobinet@me.com ?

May I get a sample file DAE that contains new exported shaders ?

I did not look in detail yet, but I see some updates for vcproj files, are the changes reflected in cmake files too ?

Thank you !

Contributor

crazyjul commented Sep 10, 2012

This is the working max 2012 exporter (I've retested this morning). I did not retested Max 2011, but we lived with both version for several months, so everything should be ok. If you want me to test it, I'll try it this week.

Most changes are just crash fixes. And of course project setup.

I'll send you a mail with such a file tomorrow when back at work
For shaders, I'll also give you a sample. The idea is to export all annotated constant from HLSL shaders. We use this quite extensively.

I'm pretty sure nothing was reported to cmake. I'll take a look and will make the fixes if necessary

Maybe I'll try to cut this pull request in smaller independent ones.

Contributor

crazyjul commented Sep 11, 2012

The only project chnaged is the max exporter. This project is not in CMake

Contributor

fabrobinet commented Sep 11, 2012

thanks for checking, I will merge locally and review more. I'll merge and/or have feedback in a couple of days approx.

Contributor

fabrobinet commented Sep 12, 2012

@crazyjul it looks that this pull request contains a fix for this one: #17 can you confirm ?

Contributor

crazyjul commented Sep 12, 2012

Yes! I've never seen that bug report, but it's exacty what we encountered

On Wed, Sep 12, 2012 at 4:35 PM, Fabrice Robinet
notifications@github.comwrote:

@crazyjul https://github.com/crazyjul it looks that this pull request
contains a fix for this one:
#17 can you confirm ?


Reply to this email directly or view it on GitHubhttps://github.com/KhronosGroup/OpenCOLLADA/pull/114#issuecomment-8495639.

Contributor

fabrobinet commented Sep 12, 2012

awesome ! thanks for the confirmation.

Contributor

fabrobinet commented Sep 13, 2012

During the last weekly COLLADA WG meeting I brought up this pull request and asked NetAllied to have a look.

We should be able to merge this quickly after next meeting. i.e next Wed.
Sorry for the delay but I prefer to have everyone well aligned for that important update.

Contributor

crazyjul commented Sep 13, 2012

no problem, take the time it takes to get it right

Contributor

fabrobinet commented Sep 14, 2012

I tested the framework part (even though it's clearly not where most changes are) rebuilt everything from scratch and converted a file with COLLADA2JSON based on that pull request without issue.

Contributor

fabrobinet commented Sep 17, 2012

@opencollada-sebastian did you have the chance to have a look at this pull request ? Would be great to have some feedback by Wed (before COLLADA weekly call). Just an overview feedback would be good to start with, I can look at the implementation details.Thanks.

@crazyjul crazyjul commented on the diff Sep 17, 2012

COLLADAMax/src/COLLADAMaxEffectExporter.cpp
@@ -268,17 +271,24 @@
if ( effectFileName && *effectFileName )
{
+ /*
@crazyjul

crazyjul Sep 17, 2012

Contributor

@fabrobinet That might explain why some stuff are missing now. Can you confirm?

@fabrobinet

fabrobinet Sep 17, 2012

Contributor

@crazyjul Just had a look, that looks wrong indeed.
That said, to which stuff are you referring to ? the email I sent you or some GitHub issues ?

From my understanding, this code removed the export of the common profile when a IDxMaterial holds a valid filename (to just export HLSLProfile). I believe this can an issue because not everyone will support that, and fallback to common profile should always be provided for better interoperability. So I would uncomment this and allows the export of both profiles. What do you think ?

Also, some commented out code has been added (see after line 308 for instance ), could it be removed or is it here because of a work in progress ?

@crazyjul

crazyjul Sep 17, 2012

Contributor

I was referring the email.

I think that both profile should be exported, but as far as I can remember, you can't setup two material in max, so what would you export?

I haven't wrote that code. I'll ask the guy that worked on tomorrow.

Contributor

fabrobinet commented Oct 2, 2012

I believe if that case only happens for directs materials then it is fine because (looking at the SPEC) at least one profile must appear but not necessarly the common profile.
So to make this more handy we could add once this is pushed an issue to track a fallback to a common profile (even a dummy one) so that every client get a change to have some valid data.

Contributor

fabrobinet commented Oct 2, 2012

@opencollada-sebastian Sebastian, sorry to insist, but we have the weekly call tomorrow can you do a quick check by then ? that would be great input to have and while I am mostly fine with this request I prefer to have you a bit involved as it might impact your upcoming work from there.

Contributor

opencollada-sebastian commented Oct 3, 2012

Everything compiles just fine here and the coding style matches the existing code base. Great work! We do have bits of untested and uncommitted work here that needs further verification and the code style needs some thorough cleanup.

Contributor

fabrobinet commented Oct 3, 2012

@opencollada-sebastian Excellent, thanks to have looked into this.
The untested code is typically here for new features so I guess so there is low risk to break anything right ?
Regarding the cleanup and code style do you suggest we should do it before merging ? If yes is there specific parts you want Julien to refine or do you plan cleanup after we merge this ?

Contributor

opencollada-sebastian commented Oct 3, 2012

I'd suggest to merge the new stuff right now and do a cleanup later.

Contributor

fabrobinet commented Oct 3, 2012

That's good with me too. For COLLADA2JSON I lived with that OpenCOLLADA version (including that patch) for several weeks without issue.

Merging now.

@fabrobinet fabrobinet added a commit that referenced this pull request Oct 3, 2012

@fabrobinet fabrobinet Merge pull request #114 from FishingCactus/Max2012
3DSMax fixes + support for 2012
b2a151a

@fabrobinet fabrobinet merged commit b2a151a into KhronosGroup:master Oct 3, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment