Tracktion / tracktion_engine

Tracktion Engine module
Other
1.2k stars 152 forks source link

Cmake support #49

Closed haras-unicorn closed 3 years ago

haras-unicorn commented 3 years ago

What I did: I just copied the style of JUCE to add CMake support, copied JUCE CMake examples with the "Tracktion" prefix for targets, built them with juce::tracktion_engine and juce::tracktion_graph, included those headers in the source files and everything seems to be working fine. I didn't run the tests because the tests require VS2017 and I have VS2019, so hopefully, this is ok.

Bugs encountered: There was a bug with the latest JUCE version where the method audioProcessorChanged of ExternalPlugin needed an additional parameter const ChangeDetails&, so I just added it in there, so hopefully, that doesn't break anything else. It just seems like an additive feature and not a subtractive one, so I guess it is ok.

Other information: My JetBrains CLion IDE added some stuff in there as well, so I added JetBrains .gitignore stuff. Everything I did is in a new branch called cmake, so if you accept the pull request the master branch is not going to be affected. EDIT: I just realized this is not the case, so I'm sorry about that. I didn't add any licensing to the new files, so you might want to do that first.

haras-unicorn commented 3 years ago

Btw this is my first pull request with a fork, so sorry for being a noob.

drowaudio commented 3 years ago

Before I go through this in too much detail, have you seen the tracktion_graph branch? https://github.com/Tracktion/tracktion_engine/tree/tracktion_graph/examples That has cmake examples (in the folders) for all of the existing examples and only required minimum changes to the code. It also uses cmake to run the builds and tests now.

We will be merging tracktion_graph in to develop/master when the new engine becomes the only engine so I don't really want to make changes to other branches that could conflict with this.

Does this branch do everything you need?

Also, rather than adding CLion files to the repo, you should probably just use a local folder for your project files so they're outside.

haras-unicorn commented 3 years ago

I have seen a bit of the tracktion_graph branch and it seems that the code is roughly the same without the root CMakeLists.txt. It would be nice if that branch added the root CMakeLists.txt as well, so people can use CMake FetchContent easily on it.

This branch does everything I need for now, so I will be using this one until you merge tracktion_graph to master.

Sorry for the CLion files, I will remember not to add IDE stuff in fork pull requests in the future (for all repos in general).

drowaudio commented 3 years ago

Ok, I'll try and add that to the tracktion_graph branch when I get a moment. Thanks

drowaudio commented 3 years ago

This PR is a bit big and I it seems to include a lot of unnecessary changes now. Would it be possible to close this one and create a new one that only has the CMakeLists.txt changes you mentioned for FetchContent to work?

haras-unicorn commented 3 years ago

I'm sorry for the notification! I just needed to build examples for my project, so I pushed it to my branch not knowing it would be added here as well. I could remove all the changes that don't involve getting FetchContent to work in this branch and use another in my fork where I have examples added for my project, so only the top-level CMakeLists.txt would remain and the stuff you mentioned that was in the tracktion_graph branch. One question, though, I'm using CLion, so can I just add the ".idea/" folder (CLion uses that folder for everything) to .gitignore, so I don't have to remove those files on my machine?

drowaudio commented 3 years ago

It just doesn't feel like CLion should be putting anything in the tracktion_engine module folder? Is this only because you're opening the examples with CLion? In that case, maybe it's better to just make a local copy of the PIP? Or just make sure you don't commit those files?

I'm just worried if I gitignore them it will be problematic if someone add files with a similar naming scheme or we actually want to include some CLion files at some point...

haras-unicorn commented 3 years ago

Yes, I was opening the whole project with CLion, so it added the ".idea/" folder in the top-level directory. Ok, no worries. I will just remove those files then and push when I remove everything except the top-level CMakeLists.txt and the things you mentioned in the tracktion_graph branch (I believe it's just one CMakeLists.txt in the modules folder). Is that okay?

drowaudio commented 3 years ago

Yeah, that would be best, just gives me a clearer idea of what's needed and what isn't. You might be able to tell CLion to put the idea files somewhere else. I've not really used it though.

drowaudio commented 3 years ago

I've added the relevant CMake changes here now: https://github.com/Tracktion/tracktion_engine/blob/develop/CMakeLists.txt https://github.com/Tracktion/tracktion_engine/blob/develop/examples/CMakeLists.txt

But to be honest I'm still not sure how these are supposed to be used an if I've done it correctly. I would expect to be able to just type cmake -G Xcode -B ~/te_build and have all the IDE files generated but that's not really what I get, just a lot of juce include errors. I think this is a bit beyond my CMake knowledge.

drowaudio commented 3 years ago

I'm going to close this now as I think everything is covered? If not, create a new issue/PR so we can discuss specifics.