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

Load an arbitrary file with AudioIn in a simulator. #1390

Closed
stc1988 opened this issue Aug 14, 2024 · 4 comments
Closed

Load an arbitrary file with AudioIn in a simulator. #1390

stc1988 opened this issue Aug 14, 2024 · 4 comments

Comments

@stc1988
Copy link
Contributor

stc1988 commented Aug 14, 2024

Build environment: macOS
Moddable SDK version: c40c945
Target device: desktop simulator

Steps to Reproduce

  1. cd $MODDABLE/examples/pins/audioin/replay and add resource that I want to load to manifest.json.
"data": {
	"sim": "$(MODDABLE)/examples/assets/sounds/wilhelm-scream"
}

2 Build and install the app using this build command: mcconfig -d -m
3. Play bflatmajor

Other information

I confirmed data section of $MODDABLE/build/tmp/mac/mc/debug/replay/manifest_flat.json.

 "data": {
        "sim": [
            "/Users/satoshi/Projects/moddable/examples/assets/sounds/bflatmajor",
            "/Users/satoshi/Projects/moddable/examples/assets/sounds/wilhelm-scream"
        ]
},

Key sim has multiple value and $MODDABLE/build/tmp/mac/mc/debug/replay/resources/sim.wav is bflatmajor.

I’m not sure if the manifest is being loaded correctly, but being able to load arbitrary files would make it more convenient to develop features using AudioIn.

@phoddie
Copy link
Collaborator

phoddie commented Aug 14, 2024

I think this issue is a feature request to provide a way to override the default WAVE file used by audio input simulator.

Today the only way to do that is to modify the path in $MODDABLE/examples/pins/audioin/replay/manifest.json.

You tried overriding that path by modifying the path in the replay example. That didn't give the expected result because of the way mcconfig merges the manifests. I don't think we should try to change that in mcconfig because it could easily have unintended side-effects that could cause problems with other projects.

One easy approach would be to provide an option to the constructor to pass the name of the resource. There are currently no arguments to the constructor, so this will not cause any compatibility issues:

	constructor(options) {
		this.#wav = new Resource(options?.resource ?? "sim.wav");		

The disadvantage of this approach is that now the calling code includes knowledge of the simulator which is misleading when running on the device:

new AudioIn{{resource: "wilhelm-scream.wav"});

An alternative is to use a config value set in the project manifest:

	constructor(options) {
		this.#wav = new Resource(config.audioInWave ?? "sim.wav");		

What approach do you think makes the most sense?

@stc1988
Copy link
Contributor Author

stc1988 commented Aug 15, 2024

I think this issue is a feature request to provide a way to override the default WAVE file used by audio input simulator.

Sorry for the confusion with the question, but this is a feature request.

One easy approach would be to provide an option to the constructor to pass the name of the resource. There are currently no arguments to the constructor, so this will not cause any compatibility issues:
The disadvantage of this approach is that now the calling code includes knowledge of the simulator which is misleading when running on the device:

Yes, I agree. It would be better if there were source code compatibility between the simulator and the device.

Therefore, using a config value might be more convenient, as it allows the project to override the settings.​

@phoddie
Copy link
Collaborator

phoddie commented Aug 15, 2024

No problem. I agree that the config approach makes the most sense here. I'll commit that.

FYI – I noticed that the replay example didn't play the default sound correctly. That is because the source data is stern, which the audioIn module and the replay example didn't handle correctly. I've got fixes for that as well (not so important as stereo audio input on embedded is relatively rare, but... the example should work correctly.

@phoddie
Copy link
Collaborator

phoddie commented Sep 14, 2024

Fixed.

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

No branches or pull requests

2 participants