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

Simple command to create MIX files #6713

Merged
merged 0 commits into from Nov 18, 2014

Conversation

Projects
None yet
5 participants
@dan9550
Copy link
Contributor

commented Oct 8, 2014

it's on my bleed branch by accident lol and YES IM ALIVE AGAIN :D

Anyway credit to @ScottNZ for line 20. Also think that some extra validation wouldn't go astray and an extract command in the future possibly.

@dan9550 dan9550 force-pushed the dan9550:bleed branch from 9137beb to 1e49dc7 Oct 8, 2014

@Phrohdoh

This comment has been minimized.

Copy link
Member

commented Oct 8, 2014

Why are we wanting to create MIXs now?

@dan9550

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2014

Well i wanted to create a mix with wav files for scores instead of aud files, personal use :)
Just something handy to have i think.

@Mailaender

This comment has been minimized.

Copy link
Member

commented Oct 10, 2014

We support it since 974e9b3 actually almost since the beginning https://github.com/OpenRA/OpenRA/commits/26cbb9d9c64b6f40a8c8c6e0ee7a4df840355b06/OpenRA.FileFormats/PackageWriter.cs and this exposes it to the users which is a good thing.

@Mailaender

This comment has been minimized.

Copy link
Member

commented Oct 11, 2014

mono --debug OpenRA.Utility.exe ra --create-mix lua.mix ./lua/
MIX archive lua.mix created successfully

image

👍 works!

}
catch
{
Console.WriteLine("An error occoured while attempting to create MIX archive " + args[1] + ".");

This comment has been minimized.

Copy link
@Mailaender

Mailaender Oct 11, 2014

Member

This swallows too much for my taste. The whole stacktrace is a little more informative than this sentence with just repeating one parameter. Just remove the try-catch block here.

@dan9550

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2014

I think i've done one better ;)

stacktrace

@pchote

This comment has been minimized.

Copy link
Member

commented Oct 11, 2014

I'm not a big fan of suppressing the exception message / type, which can be just as useful as the stacktrace in some cases.


Console.WriteLine("Your MIX archive " + args[1] + " was created successfully.");
}
catch (Exception ex)

This comment has been minimized.

Copy link
@Mailaender

Mailaender Oct 15, 2014

Member

The whole try catch is superfluous and will circumvent utility.log. Please remove it completely as I suggested in the first place.

@pchote

This comment has been minimized.

Copy link
Member

commented Oct 18, 2014

The local mix database that is written into the created mix isn't always parsed by xcc mixer, which IMO defeats the main purpose of this. We should investigate and fix that before merging this. The test posted by @Mailaender above is one of the problematic cases.

@dan9550

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2014

I wonder whats causing that..

@dan9550 dan9550 force-pushed the dan9550:bleed branch from 9b280c8 to 143db93 Oct 19, 2014

@Mailaender

This comment has been minimized.

Copy link
Member

commented Nov 8, 2014

👍 on the utility command without the superfluous try/catch constructs. The non-parsed XCC mixer database should be filed in a separate bug report.

@pchote

This comment has been minimized.

Copy link
Member

commented Nov 16, 2014

I don't see a usecase for this feature while it doesn't support the defacto standard mix databases. OpenRA doesn't need mix files, and the original-game modding community won't want to use it if it isn't interoperatble with their other tools.

@Mailaender

This comment has been minimized.

Copy link
Member

commented Nov 16, 2014

We otherwise don't have a test case for MIX file write support which is probably the reason it stayed broken for so long.

@dan9550 dan9550 force-pushed the dan9550:bleed branch from 143db93 to 0f591bb Nov 18, 2014

@Mailaender Mailaender merged commit 0f591bb into OpenRA:bleed Nov 18, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@pchote

This comment has been minimized.

Copy link
Member

commented Nov 18, 2014

If nobody cares enough about this feature to fix it (or otherwise show interest in using it) before I get around to reworking our virtual filesystem code, then I will remove it along with the rest of the otherwise dead mix-writing code. Its not really fair to force this (assuming no interest) unused and extremely crufty code to be rewritten before we can give that part of the engine a much needed improvement.

@obrakmann

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2014

What happened here? The commit and changed files lists are empty, there's no revert button, the merge commit doesn't show up in https://github.com/OpenRA/OpenRA/commits/bleed, and the commit itself isn't in Dan's bleed branch anymore, either.

@Mailaender

This comment has been minimized.

Copy link
Member

commented Nov 18, 2014

I never pressed the merge button. I guess @dan9550 just rewrote his bleed branch and all his changes including this pull request are lost. Next time use git branch cool-new-feature @dan9550.

@Mailaender

This comment has been minimized.

Copy link
Member

commented Nov 18, 2014

Go ahead and purge the MixWriter code then if this delays your Tiberian Sun efforts @pchote. There are also some out-commented lines in the OpenRA.TilesetBuilder.exe which can go away, too.

@dan9550

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2014

ahh damn, forgot i was stupid enough to put this on bleed :
Seems like it wasn't going to be merged anyway, can always make this util command again when it will work properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.