Make ozz work with vs2017 and fbx2019

m0ppers commented 5 years ago

Apparently vs2017 didn't increase the major version of cl.exe.
at least on windows after generating and trying to build I am getting dozens of linker errors. the sdk contains bundled versions for xml and zlib. I have modified the cmake file to include them as well for msvc.
the LIBS_DEBUG stuff was unused and I removed it

guillaumeblanc commented 5 years ago

Thanks for your work and sending a PR, very useful!

I added a few comments to your changes. Could you look at them?

Few questions also:

Other platforms (linux, macos...) need to link with xml2 and zlib libraries also, but you can use those provided with the system. Maybe the find_library will still work (TBC), but it should be moved out of the "if msvc" scope.

2019.1: Starting with this version of the FBX SDK your programs must explicitly link with the zlib and libxml2 libraries. On Linux and MacOS, these libraries should already be part of the system. If not, you must install them.

Continous build ckecks (appveyor and travis) need to be updated. Can you add at least a build with 2019.2 version of fbx sdk? I'll rework them then to test different versions.
Do you know if I misused PARENT_SCOPE? Maybe the parameter passing strategy I use is not the correct one for cmake function?

Cheers, Guillaume

m0ppers commented 5 years ago

Stupid question: Where would I find these comments? :D Thought you added some comments to the commit but I don't see anything?

To your questions:

ahh...so it seems I was the first to build with the 2019 edition :D makes sense. So it is probably worthwhile to invest a bit more time into generalizing this. Will rework this to make it work for unix systems as well. Give me some more time. Probably will find some time on the weekend. this will probably be a bit bigger then (wanted to change only the necessary things to make it build for me so far).
yes. I will update the CI builds as well while I am at it.
puhhh...I am not a real CMake expert (is there anyone at all? :D). in the CMake builds I maintained so far PARENT_SCOPE was not unusual. so I guess it is ok? will have a detailed look once I rework this and update this PR.

guillaumeblanc commented 5 years ago

Hi,

I didn't published the comments (and even lost them). So I typed them again. There's no stupid question ;)

Thank you for considering CI and unix, that's really helpfull! No worries for the timing.

Cheers, Guillaume

m0ppers commented 5 years ago

Some update: hope I got everything?

One more comment: I had problems getting it to build on MacOS: First OpenGL was deprecated (can be fixed with -DGL_SILENCE_DEPRECATION (or something like that) and apparently at least the new fbx sdk does some #pragma pack stuff which is conflicting with some mac internal stuff...the official examples also throw a warning but not an error so they build. I could fix it by adding -Wno-pragma-pack not sure if that should be added to the cmake stuff.

guillaumeblanc commented 5 years ago

Hi,

I have a good and a bad news. Good news first ;) ?

The Good is that for MacOS I already fixed the problems you reported (opengl and pragma pack) in "develop" branch. I'd suggest to move your PR to that branch, as it'll soon be pushed to master anyway.
The bad: Seems the changes on the CI side are not yet ok, as fbx sdk download is failing. Something wrong with the env variable apparently. As a result the sdk isn't found.

Thanks for the hard work, it's not that fun to maintain! I'll help if you can't find the time. That PR is very useful indeed !

Guillaume

m0ppers commented 5 years ago

oops....will fix...should have had a look at the logs....

m0ppers commented 5 years ago

Ok that was much more difficult than expected but I think it is working now. Can you check?

guillaumeblanc commented 5 years ago

Hi,

It was difficult indeed as many things changed with this sdk (chmod, xml, zlib...). Thanks for your perseverance!

It works great! I just found that it was not possible anymore to use a old fbx sdk with vs2017: https://ci.appveyor.com/project/guillaumeblanc/ozz-animation/build/job/mktfh1nh7fti4em0?fullLog=true#L61 . It's due to sdk (compiler) folder name that was vs2015 before this sdk (not vs2017). I don't see how to fix it in a simple way. It can probably remain like that, unless you have an idea?

I'll integrate your PR to develop asap, it's already in a branch. Thanks a lot for your help!

Cheers, Guillaume

m0ppers commented 5 years ago

don't see a super simple way but the HINTS https://github.com/m0ppers/ozz-animation/blob/hotfix/vs0217-and-fbx2019/build-utils/cmake/modules/FindFbx.cmake#L107 can be a list so we could restructure the stuff before that to allow searching multiple paths for vs2017 but maybe that should be done in a different branch once everything is merged. I think it makes things even more complicated now that you have integrated on a different branch.

m0ppers commented 5 years ago

or I could create a new PR to your branch once you are done with your adjustments. what do you think?

guillaumeblanc commented 5 years ago

Yes that would require a PR to the new branch. I'm afraid it will complexify the code: see how FBX_SEARCH_LIB_PATH is used, especially with debug/release added combinatory. Don't know if PATH_SUFFIXES can help in that regard. I'm fine if you want to try, but don't bother too much about it. I'm not sure it's worth it.

m0ppers commented 5 years ago

ok won't do it then :)

guillaumeblanc commented 5 years ago

It's in now. Thanks a lot for your work!

guillaumeblanc / ozz-animation

Make ozz work with vs2017 and fbx2019 #64