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

Add bank aliasing and case insensitivity #1245

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

TodePond
Copy link

@TodePond TodePond commented Jan 18, 2025

What is this pull request?

This is a spicy pull request that adds an aliasBank function. It lets you create an alias for any bank of sounds.

image

This pull request also adds lots of aliases by defaults. For example, "TR909" is an alias for "RolandTR909". Check out the aliases JSON file for all the aliases.

image

This pull request also removes cases-sensitivity from ALL SOUNDS. This is a big breaking change.
If this isn't wanted at this point, let's remove it and discuss within a future pull request.

image

Release notes

  • Added the aliasBank function that lets you create aliases for banks.
  • Added lots of default aliases for drum machine banks, eg: "TR909" for "RolandTR909".
  • Sounds are now case insensitive.

Future work

  • Make the sounds interface nicer and easier.
  • Move the JSON hosted elsewhere into the strudel repo (see comments).

@yaxu
Copy link
Member

yaxu commented Jan 18, 2025

How does this look in the sound browsing interface?

I find the sound bank implementation difficult to teach in workshops, especially where 'sounds' are already a sample set (via n) and 'banks' are a level above but don't appear that way in the sound browser. Improving this is probably out of scope for this PR but we should try not to make things more confusing.

Agreed the shorter names are better and would like to deprecate the long ones. Perhaps we could also go for all lowercase with the short versions.

@felixroos
Copy link
Collaborator

How does this look in the sound browsing interface?

I find the sound bank implementation difficult to teach in workshops, especially where 'sounds' are already a sample set (via n) and 'banks' are a level above but don't appear that way in the sound browser. Improving this is probably out of scope for this PR but we should try not to make things more confusing.

Agreed the shorter names are better and would like to deprecate the long ones. Perhaps we could also go for all lowercase with the short versions.

yes the sound tab needs renovation!

another hot take hot take hot take: we could make use of the fact that all drum samples get new names and give a little tlc to the samples (e.g. 808 defaults are weird) + maybe mix them differently (hihats quieter etc..) ? if we do it now, before people use them, we won't get a breaking change..

@yaxu
Copy link
Member

yaxu commented Jan 18, 2025

another hot take hot take hot take: we could make use of the fact that all drum samples get new names and give a little tlc to the samples (e.g. 808 defaults are weird) + maybe mix them differently (hihats quieter etc..) ? if we do it now, before people use them, we won't get a breaking change..

Yes that's a nice idea! Then every time we have to type RolandTR808 we're more motivated to do some sample mastering..

@yaxu
Copy link
Member

yaxu commented Jan 18, 2025

Strictly speaking Visco probably isn't a manufacturer name because Visco Space Drum probably never existed https://gearspace.com/board/electronic-music-instruments-and-electronic-music-production/390269-visco-space-drum.html

@TodePond
Copy link
Author

How does this look in the sound browsing interface?

good point, i hadn't thought of that. thanks for pointing it out.

this pr could probably filter out one set of names in the menu, and a future pull request could improve the overall ux of the drum sound menu

Perhaps we could also go for all lowercase with the short versions.

even better could be to make it case insensitive?

Visco Space Drum probably never existed

woah!

I'm not wedded to any choices here, will happily merge or make any changes no questions asked


just a heads up to both of you that the shortened list misses off a couple that i couldn't figure out how to shorten

@felixroos
Copy link
Collaborator

felixroos commented Jan 22, 2025

an alternative take on this would be to handle aliases after the json has been processed. here's an example aliasBank function . this would save file size + it's usable with from disk autogenerated strudel.json files á la @strudel/sampler (based on discussion in strudel-innards)

@felixroos felixroos mentioned this pull request Jan 22, 2025
17 tasks
@felixroos felixroos added the v1.2 label Jan 24, 2025
@felixroos
Copy link
Collaborator

@TodePond do you want to add the aliasBank function above? would be nice to get this into the next version

@TodePond
Copy link
Author

sure thing! do you mean just the function? (for people to use)

or also all the aliases? (called with the function)

hope that makes sense

@felixroos
Copy link
Collaborator

sure thing! do you mean just the function? (for people to use)

or also all the aliases? (called with the function)

hope that makes sense

i'd say also the aliases called with the function. maybe make them lowercase as well? there might also be a way to improve / simplify it for setting up multiple aliases at once but not super important

@TodePond
Copy link
Author

yes!

when are you planning / hoping to do the release by the way?

@felixroos
Copy link
Collaborator

yes!

when are you planning / hoping to do the release by the way?

soonish.. when the next wave of PRs is merged, but not sure when that will be exactly..

@TodePond TodePond marked this pull request as draft January 26, 2025 12:38
@TodePond TodePond changed the title [hot take] Add shortened bank names Add bank aliasing and case insensitivity Jan 26, 2025
@TodePond TodePond marked this pull request as ready for review January 26, 2025 16:53
@@ -21,6 +21,9 @@ export async function prebake() {
);
// load samples
const ds = 'https://raw.githubusercontent.com/felixroos/dough-samples/main/';

// TODO: move this onto the strudel repo
const ts = 'https://raw.githubusercontent.com/todepond/samples/main/';
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -36,6 +39,8 @@ export async function prebake() {
samples(`${ds}/EmuSP12.json`),
samples(`${ds}/vcsl.json`),
]);

aliasBank(`${ts}/tidal-drum-machines-alias.json`);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the bet way to try out the repl? i only checked the web version works (with pnpm dev)

Copy link
Collaborator

@felixroos felixroos Jan 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i test with this https://github.com/tidalcycles/strudel/blob/main/examples/buildless/web-component-no-iframe.html replacing the unpkg url with the locally built repl package (run pnpm build in the repl folder)

return aliasBankMap({ [args[0]]: args[1] });
default:
throw new Error('aliasMap expects 1 or 2 arguments, received ' + args.length);
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if theres a better place for this function to live lemme know and i'll move it

"RolandTR707": "TR707",
"RolandTR727": "TR727",
"RolandTR808": "TR808",
"RolandTR909": "TR909",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kinda wanna add "909" as another alias but its a slippery slope

website/public/tidal-drum-machines-alias.json Outdated Show resolved Hide resolved
"SimmonsSDS5": "SDS5",
"SoundmastersR88": "R88",
"UnivoxMicroRhythmer12": "MicroRhythmer12",
"ViscoSpaceDrum": "SpaceDrum",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its real my neighbour told me

@TodePond
Copy link
Author

ready for review now!
Read the pull request description for the current changes.
left some comments too

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

Successfully merging this pull request may close these issues.

3 participants