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

NScumm.Audio.Players classes should be public #8

Open
BenMcLean opened this issue Dec 5, 2020 · 6 comments
Open

NScumm.Audio.Players classes should be public #8

BenMcLean opened this issue Dec 5, 2020 · 6 comments

Comments

@BenMcLean
Copy link
Contributor

Right now, the NScumm.Audio.Players classes are marked internal and sealed. They should all be public instead.

@scemino
Copy link
Owner

scemino commented Dec 5, 2020

I see no reason to do that. Why do you need it ?

@BenMcLean
Copy link
Contributor Author

I see no reason to do that. Why do you need it ?

First of all, the classes shouldn't be sealed because you never know in advance how people might want to extend the classes. Classes should only ever be sealed if it's absolutely necessary for security reasons which aren't a concern here.

Second, I'm not even sure how one would instantiate an IMusicPlayer instance from outside code since none of the classes for doing so are accessible to outside code at all.

@scemino
Copy link
Owner

scemino commented Dec 5, 2020

There is a factory for that here: var players = Factory.GetPlayers(opl);.
And an example in this project: NScumm.Audio.ALPlayer.

@BenMcLean
Copy link
Contributor Author

BenMcLean commented Dec 5, 2020

Hmm ... assuming we modded this to implement issues #5 and #6 and #9 then would it be possible for this to be able to play both IMFs on channels 1-7 AND play sound effects in channel 0 like Wolfenstein 3-D does? It would need to be able to switch to different songs or to have a new sound effect interrupt the old one at any time without the sound effects interfering with the music or vice versa. I know that's kind of a tall order though.

@BenMcLean
Copy link
Contributor Author

Also, here's a case where the classes need to be exposed. Id's IMF songs can play back at 700hz or 560hz depending on what game engine the IMF was written for. There is absolutely nothing in the file format itself to indicate what speed to play it back at. It is also perfectly valid for other games to use the IMF format and play it back at any arbitrary playback rate.

Adplug uses the ".imf" file extension to indicate 560hz and the ".wlf" file extension to indicate 700hz but that's an arbitrary convention made up by Adplug and that information isn't communicated if we're sending in a byte stream instead of a file.

scemino added a commit that referenced this issue Dec 7, 2020
Fixes #5 and #8
ImfPlayer is exposed in order to customize its rate
@BenMcLean
Copy link
Contributor Author

The demands of re-implementing Wolfenstein 3-D eventually led me to skip using NScumm.Audio.Players in my app altogether and only use NScumm.Core.Audio.OPL. https://github.com/BenMcLean/WOLF3D-Godot/tree/4d5f895b8d54ae2d9b8a616c434cde3f9441247c/godot/WOLF3DGame/OPL

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

No branches or pull requests

2 participants