Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streamline CLI Access #19

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open

Streamline CLI Access #19

wants to merge 31 commits into from

Conversation

Jan-Ka
Copy link

@Jan-Ka Jan-Ka commented Jan 15, 2024

Hello,

I want to use this tool in a different project and needed the ability to provide:

  • CWD (no input path) to Output Filepath
  • Input Filepath to Output Filepath
  • Input Folder to Output Folder
  • Input Filepath to Output Folder

Some of these combinations the current CLI Args Handling did not seem to support. I'm relearning C/C++ and had a bit of a hard time understanding the current implementation (to add just that), so instead I tried to rework it until I could.

Just want to know if this approach would be useful for this project. It's now at a good spot to drop it if it isn't or if this isn't the way it is properly done in c/c++.


I'd like to get a go ahead because I couldn't establish if my variant works, as I can't seem to export soundbanks from W-wise that return any information. (They also don't seem to work with the latest release of wwise-audio-tools)

I've attached my test data. Everything is made in W-wise 2019.2.15.7667 (because that's what CP2077 needs to work, not sure if a newer/older version matters). Test.bnk is a custom demo file from a new project; l_intro.bnk and l_testbank.bnk are exported from the Limbo Samplepack from Audiokinetic.

Wasn't sure about what the proper etiquette for adding libraries in c/c++ projects is. https://github.com/jarro2783/cxxopts seems to be a common low-hanging fruit. It did not meaningfully impact build time or size of the EXE on my machine.

Thanks for your time,
JK

testdata_bnk.zip

@abheekda1
Copy link
Collaborator

abheekda1 commented Jan 16, 2024

Thank you for the PR! I think it looks pretty good, I will test it out and take a look at your test data. I think I will probably add the cxxopts library as a git submodule, but other than that I think it's good.

@abheekda1
Copy link
Collaborator

For the Soundbanks I've taken a look, seems like there's an issue on my part but I'm a little confused. bnkextr seems to work, but the definition on Xentax seems to be different than the implementation for bnkextr. I've pinned it down to

- id: event_action_count
type: u4
where bnkextr uses an unsigned integer of size 1 byte whereas Xentax defines an unsigned integer of size 4 bytes. I will try to test some Soundbanks from the Witcher 3 to make sure it's not a discrepancy between Wwise versions (I think the Witcher 3 uses some early 2010s version), and if that fixes it I will go ahead and make that change.

@abheekda1
Copy link
Collaborator

It seems like there was a switch between u1 and u4 sometime between 2021 and 2023; I'll have to figure out when and account for it in the data type.

@seberoth
Copy link

They switched from uint32 to vlq in version 123

@Jan-Ka
Copy link
Author

Jan-Ka commented Jan 17, 2024

Happy that I could help and that you people like the approach.

I will attempt to finish a complete implementation today that calls all previously called code paths and move cxxopts to a submodule. If there's nothing else, it should at least allow to make comparing/testing a bit easier.

I appreciate your work.

@abheekda1
Copy link
Collaborator

They switched from uint32 to vlq in version 123

Thanks! I've updated the ksy sources in #20 and if they look good I will regenerate the parsers.

I think this PR is also close but a couple things @Jan-Ka. First of all, using any unsupported argument results in an exception, including no arguments (which I think should probably just show help). Also the help is a little bit arbitrary, I didn't quite understand it:

a simple command line tool used to convert Wwise WEM files to OGG files
Usage:
  wwtools [OPTION...] <operation> [task=value] [input file/directory] [output file/directory]

      --info
  -h, --help  Print usage

 bnk options:
      --event [=arg(=)]    (task|task=event-id)
      --extract [=arg(=)]  (task|task=event-id)
      --no-convert

 cache options:
      --read            (task)
      --write           (task)
      --no-convert-wem
      --no-convert-bnk

(what does [=arg(=)] mean?) Assuming that it's primarily cosmetic issues and everything else is working smoothly otherwise, which I will test once I better understand the command line options, this is definitely close to being merged.

@Jan-Ka
Copy link
Author

Jan-Ka commented Jan 17, 2024

(what does [=arg(=)] mean?) Assuming that it's primarily cosmetic issues and everything else is working smoothly otherwise, which I will test once I better understand the command line options, this is definitely close to being merged.

I've checked @abheekda1 and it comes from cxxopts directly:

    // changed for testing
    ("event", "test", cxxopts::value<string>()->default_value("bla")->implicit_value("blub"))
 bnk options:
      --event [=arg(=blub)]  test (default: bla)

So, (=) seems like an edge case in cxxopts when you use an implicit_value and set it to empty string, which is required to make the flag work with and without a value.

It's not required for the use of cxxopts to have it generate the help. We can override it through cxxopts or just ignore it (e.g. implement it like you did it previously, directly)

Would be cool/good if we could still provide .editorconfig and .clang-format if you prefer those styles. I have no hard feelings either way. Without them, most IDE's just run havoc with your files when autoformatting. That's why I added them.

@Jan-Ka
Copy link
Author

Jan-Ka commented Jan 17, 2024

I'll be done with the promised changes in at most a few hours, and can fix the issues you discovered along the way.

If it's the same for you, I'd like to finish the promised work in this PR first. :)

@Jan-Ka
Copy link
Author

Jan-Ka commented Jan 17, 2024

I think I might've borked this PR by rebasing my branch.

Edit: Yeah, looks like I did. Should be fine if we squash before merging.

@abheekda1 abheekda1 marked this pull request as ready for review January 17, 2024 20:39
@abheekda1
Copy link
Collaborator

Other than cleaning up the exceptions and such, what else is left to be done?

@Jan-Ka
Copy link
Author

Jan-Ka commented Jan 17, 2024

Code path to cache isn't implemented at all.

Edit: bnk extract also is missing

@Jan-Ka
Copy link
Author

Jan-Ka commented Jan 17, 2024

Your recent change seems to still have an issue:

PS D:\Repos\Jan-Ka\wwise-audio-tools> cmake --build build
[  1%] Linking C static library ..\lib\libogg.a
[  3%] Built target ogg
[  4%] Linking C static library ..\..\lib\libvorbis.a
[ 28%] Built target vorbis
[ 29%] Building CXX object CMakeFiles/wwtools-shared-lib.dir/src/kaitai/kaitaistream.cpp.obj
D:\Repos\Jan-Ka\wwise-audio-tools\src\kaitai\kaitaistream.cpp:31:10: fatal error: endian.h: No such file or directory
   31 | #include <endian.h>
      |          ^~~~~~~~~~

edit: Seems to be an issue in kaitai: kaitai-io/kaitai_struct_cpp_stl_runtime#44

@abheekda1
Copy link
Collaborator

@Jan-Ka I'm curious if you still think we need to use cxxopts? The current cli tool is pretty garbage because I wrote it all at once as a proof of concept. If it works with you, I would be interested in redirecting this PR to rewrite the main.cpp file to just be a little more readable, straightforward, and simple while incorporating all your needs.

@Jan-Ka
Copy link
Author

Jan-Ka commented Feb 3, 2024

@Jan-Ka I'm curious if you still think we need to use cxxopts? The current cli tool is pretty garbage because I wrote it all at once as a proof of concept. If it works with you, I would be interested in redirecting this PR to rewrite the main.cpp file to just be a little more readable, straightforward, and simple while incorporating all your needs.

I'm not married to cxxopts at all, it was just the quickest solution to the problem I had. Shouldn't be too much work, if you just parse out the few elements that are actually needed.

Sorry, Life got in the Way of this PR pretty severely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants