jar support for ogg loader

[divzero]

[quote]Changes are committed now.

It’s good that you said this Yuri, because I wanted to ask how to handle format detection with InputStreams anyway. Don’t know why I didn’t do it. I added also some documentation and error handling. If it is not correct please tell me (via private message if it’s a specific thing). Code formatting, error handling and logging conventions would be useful in my opinion.
[/quote]
I agree, code formatting error handling and logging convenstions would be a very good thing. I tried to get a discussion on the topic ages ago in a thread but never really got anywhere.

The code works - but some nitpicking:

• If the file is not found, can a FileNotFoundException be thrown with a message rather than a NPE? If not FNFE - then some other checked exception because this is really something that needs catching!

• I notice that the code dosn’t attempt to load the files relative to the working directory but only the class path. I guess this isn’t a large problem but dual support would be nice.

I wounder if we should setup the sounds like the textures with a register path type of idea?

Just my $0.02

Will.

[joo]

[quote]I agree, code formatting error handling and logging convenstions would be a very good thing. I tried to get a discussion on the topic ages ago in a thread but never really got anywhere.
[/quote]
This probably needs to be discussed in a seperate thread, if more devs cry for it.

[quote]* If the file is not found, can a FileNotFoundException be thrown with a message rather than a NPE? If not FNFE - then some other checked exception because this is really something that needs catching!
[/quote]
A FileNotFoundException is thrown if you pass a wrong filename. What I can imagine you are refering to is when you load an URL using getClass().getClassLoader().getResource(“foo.sound”). In this case this returns null if the resource is not found. I can check for a null URL in the constructor of MediaContainer and return an error if this is the case. I agree this would be useful and easy to add. (Disclaimer: This issue was not introduced by my changes in the code. In fact I added a lot of error handling.)

[quote]* I notice that the code dosn’t attempt to load the files relative to the working directory but only the class path. I guess this isn’t a large problem but dual support would be nice.
[/quote]
You mean I should load from the current directory when loading an ogg file from an URL? I think is always done unless you explicitly exclude the current directory from your classpath. (Correct me if I understood this wrong.)

[quote]I wounder if we should setup the sounds like the textures with a register path type of idea?
[/quote]
This is a bit more complicated than with textures, because we have different sound loaders, which would have to implement this functionality (because the loading mechanisms are different). But of course it’s possible.

[divzero]

A FileNotFoundException is thrown if you pass a wrong filename. What I can imagine you are refering to is when you load an URL using getClass().getClassLoader().getResource(“foo.sound”). In this case this returns null if the resource is not found. I can check for a null URL in the constructor of MediaContainer and return an error if this is the case. I agree this would be useful and easy to add. (Disclaimer: This issue was not introduced by my changes in the code. In fact I added a lot of error handling.)

I think that a FNFE would be suitable when that’s it’s null to avoid a less meningfull NPE. Sorry to confuse this was just a request, not a comment on your changes

I don’t really care what Exception it is but it should be a) checked and b) carry a message.

You mean I should load from the current directory when loading an ogg file from an URL? I think is always done unless you explicitly exclude the current directory from your classpath. (Correct me if I understood this wrong.)

Actually it doesn’t check the current working directory as far as I can see.

From the demo directory:
java -cp …/libs/xith3d.jar com.xith3d.test.JavaSound NPE’s

java -cp …/libs/xith3d.jar:. com.xith3d.test.JavaSound works

The difference in the second is that you are adding the current directory into the classpath which is more friendly with getResource(). So the code only loads the image from the classpath - I was suggesting adding support for the current working directory as well but it’s not all that important.

This is a bit more complicated than with textures, because we have different sound loaders, which would have to implement this functionality (because the loading mechanisms are different). But of course it’s possible.

I see, probably a waste of time then.

Cheers,

Will.

[joo]

[quote]I think that a FNFE would be suitable when that’s it’s null to avoid a less meningfull NPE.
[/quote]
I added a check to MediaContainer, which throws an error if you pass null as URL. If this was not what you talked about, then please explain it in more detail. As I said above calling new MediaContainer(getClass().getClassLoader().getResource(foo)) is equivalent to calling new MediaContainer((URL) null), if the resource foo is not in your classpath.

Actually it doesn’t check the current working directory as far as I can see.

From the demo directory:
java -cp …/libs/xith3d.jar com.xith3d.test.JavaSound NPE’s

That’s what I meant with “explicitly excluding the current directory”. Usually the “.” has to be there, if you specify the classpath and want to have the current directory in it. If you don’t specify a classpath loading from the current directory works. Anyway, if you want to have support for it you need to change the Java core, because the Ogg Loader only gets an URL and doesn’t search for resources.

[divzero]

I would have prefered a FNFE since it is checked and would force anyone calling the contructor to catch it. I understand this is an API change and may make some people unhappy so it’s probably best you didnt’ do it now.

Let me ask you this: Do you want your application bombing out just because it couldn’t find a particular sound? I would think in most cases - no you wouldn’t. Of course you can always catch the FNFE and exit if this is what you would like.

Unchecked exceptions are best used when the programmer has made a mistake. It’s reasonable I think that in some cases you may try to play an ogg that doesn’t exist (think if you were creating a 3D Java ogg sound player and the user can play arbituary files)

Again - just my $0.02 perhaps I should submit an RFE so that people can vote on it - it may cause a small amount of code changes.

As for the current directory - it would be possible to construct the URL by getting the current directory from “new File (.)” (rather than using the classpath) but it really doesn’t matter. If I need that function I’ll code it myself ;D It’s better practice to use the class path anyway.

Will.