e-a-h / That-Sky-Bot

That Sky [Bot] for thatgamecompany and #thatskygame
Creative Commons Attribution Share Alike 4.0 International
14 stars 3 forks source link

Dev music #37

Closed jmmelko closed 4 years ago

e-a-h commented 4 years ago

I've tried this with all branches of sky-python-music-sheet-maker and can not successfully run through the dialog. Is there a specific commit I should have checked out for this to function?

jmmelko commented 4 years ago

Hello Alex, Thanks for trying.

Actually I have created this pull request weeks ago, so in the meantime I have dropped all expectations that it would be addressed one day. So I made some refactoring in the music maker code instead.

I will straighten things. Shouldn’t take me long.

Feel free to close this pull request, I will create another one.

Thank you.

jmmelko

Le 22 mai 2020 à 03:35, eah notifications@github.com a écrit :

 I've tried this with all branches of sky-python-music-sheet-maker and can not successfully run through the dialog. Is there a specific commit I should have checked out for this to function?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

e-a-h commented 4 years ago

ok, sorry for the delayed review. I couldn't merge for a while because language refactoring was in progress. I have localization mostly functional now, and have replaced "Lang.get_string" with "Lang.get_locale_string." get_locale_string has an additional "context" parameter, which can be a discord.commands.context or a string matching one of the supported locales, or "ALL_LOCALES" for broadcasting in all languages, or blank for server default language.

Channels can now have language override, so the channel context where the dialog is initiated should set the language for the whole dialog. Command alias overrides for language are also planned, so using the command in e.g. Japanese would make the bot respond in Japanese.

jmmelko commented 4 years ago

Alex,

I have repaired the music cog. The working branch is ‘dev’.

I have not yet updated the code to match your changes on locales, but you can give it a try anyway.

Thank you.

jmmelko commented 4 years ago

OK, I have added the locale by:

I’ll leave the rest to you.

I’m looking forward to seeing the cog running before I die.

e-a-h commented 4 years ago

I’m looking forward to seeing the cog running before I die.

I guess I deserved that :D

Since android launch I've become very busy, and the bot has also become pretty busy. I've slowly working to stabilize some things and had issues with the bot crashing for a little while, so was moving slowly to avoid causing mayhem in the sever.

jmmelko commented 4 years ago

It is true that a faulty cog can make the whole bot to hang or crash, and maybe spam a channel with messages, this scares me a lot!

e-a-h commented 4 years ago

a faulty cog could make the bot crash, or could potentially just unload. but that's why I review and test :D

e-a-h commented 4 years ago

A couple problems in testing still:

e-a-h commented 4 years ago

Regarding branches, I request that the music sheet maker repo have a release-skybot branch so it's always clear what the stable version for skybot integration is. Unless the master branch is always intended to be stable for both skybot and standalone use...

And I'm not clear on how localization handoff should work between skybot-hosted language strings and those written into the music sheet maker. In terms of discord dialogs I'd like to have some control over the language presented to users, but I haven't thought through the best approach to this yet.

e-a-h commented 4 years ago

AND another thing: licensing. This may get tricky so maybe it's a discussion for another day, but might want to include license options to show permissions for sharing between sites and apps. creative commons licenses are pretty clear and easy to use, so we might start there. The biggest challenge is in applying a license to preexisting commercial music.

e-a-h commented 4 years ago

re: 10 file limit... maybe we can zip multiple files. If the 10 file limit is there because 10 attachment limit in discord, then we can also iterate and send multiple messages... but I would prefer auto-zip for more than 10. Maybe auto-zip for more than 5 even... or some reasonable number of sheets. what do you think?

If I'm understanding correctly, then I think octave shift should be disabled for sky keyboard entry styles. Entering octave shifts for almost any entry done as sky keyboard results unusable notes and red question marks.

I recommend reading through everything in the #testing channel to see ways that we broke the input. There will be many many people with poor intentions and/or poor understanding of the process, so we should try to account for some of that at least.

I'm also going to add a concurrency test to the song command so it can't be DDOS'd.

jmmelko commented 4 years ago

I don’t think your Question model allows to answer either by a reaction or a string. I mean, you have Question.ask_text which expects a string and Question.ask which expects a reaction but not a combination of the two. This would be very useful for me. Not only for aspect ratio but for other questions as well.

e-a-h commented 4 years ago

I've modeled that style of questioning in dev-art-collector branch. We can make that change, or just try address other issues to get it into production and restructure question flow later.

And I didn't dig into this, but here's an alert I got running testing: https://sentry.io/share/issue/0cdb5baa67be429eb757731139fbd4b5/

e-a-h commented 4 years ago

logging from test run yesterday if it's useful. music_logs.txt

e-a-h commented 4 years ago

I added zip file output for 4+ sheets. In testing though, it seemed I was only able to get 11 sheets total. Is there a limit in music sheet maker on how many files are output? Or how much time the output takes? I think there should be some limit set so the bot can't be intentionally hijacked with long songs, but I'm not sure what the best approach should be.

jmmelko commented 4 years ago

Yes,

I have internally set the maximum number of pages to 11. Do you want me to remove it?

I’d prefer not to.

It is very unlikely that anyone would want to generate more than 10 pages anyway.

JMM.

Le 1 juin 2020 à 15:37, eah notifications@github.com a écrit :

 I added zip file output for 4+ sheets. In testing though, it seemed I was only able to get 11 sheets total. Is there a limit in music sheet maker on how many files are output? Or how much time the output takes? I think there should be some limit set so the bot can't be intentionally hijacked with long songs, but I'm not sure what the best approach should be.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

jmmelko commented 4 years ago

‘If I'm understanding correctly, then I think octave shift should be disabled for sky keyboard entry styles. Entering octave shifts for almost any entry done as sky keyboard results unusable notes and red question marks.‘

What are you talking about?

Octave shift is NOT proposed for Sky and Sky keyboard entries.

If you encountered this, then it means either that there is a bug, or that the program detected/thought you were using another musical notation.

If so, could you please write down the steps to reproduce the bug?

Thank you.

e-a-h commented 4 years ago

I entered q w e r t etc. as my music. I'm prompted for key, and then prompted for octave shift.

jmmelko commented 4 years ago

Oops, you’re right, I had left this and nobody complained. It fixed this and aspect ratio also. I will test it locally and then pull this to a new branch as asked. I have not addressed copyright and other trick stuff yet.

As for the PNG crash I think you entered very nasty stuff in there so we’ll have to deal with it separately because it is not a systematic bug.

e-a-h commented 4 years ago

regarding max sheets: if you think 11 is a reasonable maximum based on songs you've entered or seen entered, I'm ok with that. Can the sheet maker possibly also return a message or flag if the song overruns 11 sheets though, so we can inform people when their songs are truncated?

jmmelko commented 4 years ago

Ok, here is what I have done:

You can test again using the dev branch

e-a-h commented 4 years ago

Thank you, I'll pull the latest and stand it up for testing again. I'll add concurrency test shortly.

"Arranged by" is just an "nice to have" item. I'm only thinking about interaction between other music apps and making sure we have a format of data that is portable and standardized.

Release branch is only necessary if head of master branch is not intended to be universal.

for language files, as long as we can pass locale from the bot dialog into the music sheet maker and have sheet maker respond in the same language, I'm ok. I decided on a list as return type from get_defaulted_locale because there are some channels in the server that will be universal and will need response in every available language. like the welcome channel. But bot dialog should generally only be in one language, and I might add that as question #1

Copyright is not a priority for me right now, but is mostly for future thinking, sharing between apps, and sharing outside the Sky server.

e-a-h commented 4 years ago

New issue after I pulled the latest in the dev branch. This happens when choosing a key after entering sky notation, qwert or a1b2c3. I can make it past that step if I check out the commit before.

https://sentry.io/share/issue/50396bac954e47cfb09ba3fdb54476fa/

jmmelko commented 4 years ago

Sorry, I had forgotten to push the new code for the Music Cog. (Incidentally It helped me find a serious typo in my code :-) )

JMM.

Le 2 juin 2020 à 06:13, eah notifications@github.com a écrit :

 New issue after I pulled the latest in the dev branch. This happens when choosing a key after entering sky notation, qwert or a1b2c3. I can make it past that step if I check out the commit before.

https://sentry.io/share/issue/50396bac954e47cfb09ba3fdb54476fa/

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

jmmelko commented 4 years ago

Alex,

By “master branch”, do you mean the default branch? I am asking because I am planning to name the default branch “stable” and let “master” the branch to be used by the music cog and the website, but I don’t know if it’s suitable for you.

Tx.

e-a-h commented 4 years ago

re: branches I'll rephrase. I'm not in charge of how you branch so do what makes the most sense for you; don't let me throw a wrench in your plans! As long as it's clear which commit the bot should be using, I'm ok.

jmmelko commented 4 years ago

Alex,

the branch to use is now 'master'.

What do you mean by "name entry with discord name+discriminator+maybe discord ID" Like an automatic file name: SongTitle_alexdollyazjkdnj#0001 ?

Non-blocking TODOs:

e-a-h commented 4 years ago

name entry I'm committing a semi-functional example in cogs/Music.py - I intended it to convert user id or user mention into a nice display name, but it looks like mentions are escaped or something, and I'm too tired to see where right now :) ... will fix tomorrow or so unless you want to take a crack at fixing it. For testing just the name conversion, you can use ?name_test @alexdolly (make sure the name you use completes as a mention... maybe mention yourself) and ?name_test 295762764234752000 where the number is a user id.

e-a-h commented 4 years ago

Sheets in every format appear to have one trailing keyboard. In some cases it results in extra white space. In other cases it results in the board getting cut off.

  1. should be corrected so a full sheet doesn't have one trailing
  2. maybe verify that all available aspect ratios result in nicely lined-up rows that fill the space well.

asdf_000

jmmelko commented 4 years ago

Actually, this only happens if you type a very long line, which we advised against from the very beginning. We advised to make a line break every 10 notes or so.

Recently I found a way to render long lines, it works with other output formats (such as SVG) but not well with PNG it seems.

I will fix it.

If I cannot trim I may have to add extra space for safety (ie satisfy 1. but not 2.)

e-a-h commented 4 years ago

there are no directions in the discord implementation currently that I can see indicating line breaks. I also advise against even mentioning line breaks to discord users because they will hit "enter" and prematurely end note input. then mods will get endless mentions asking how to input more music after making a line break!!

So yes, I fully endorse long-line rendering. None of my testing so far as involved using line breaks, so i should try that :D

jmmelko commented 4 years ago

OK, it’s fixed now (it was because point 0,0 in images is the top left corner, not the bottom right...)

Actually users can enter the song line by line because the program loops over the “enter notes” question, but I agree that this implicit and confusing.

t1-tracey commented 4 years ago

What’s the status of this?

e-a-h commented 4 years ago

What’s the status of this?

At the moment it's blocked by some other tasks I have for fixing things in the bot. I'll get back onto it as soon as I can.

jmmelko commented 4 years ago

Great job. It will take me several days to believe this is actually happening :-))

Just a question: will the bot be only triggered by `!song' ? Can we add listening keywords to invite users to use the maker? I mean, just like when the bot says something about Eden rest when someone mentions Eden. I don’t want to spam the forum with bot messages, but I thought that maybe some kind of subtle, soft advertising might be necessary. At least on the Sky-music channel.

e-a-h commented 4 years ago

music sheet maker is live in sky official server now. you can try it yourself.

at the moment, !song is the only trigger. it's possible to add autoresponders or commands with instructions, but I don't want to push it too hard at first in case it runs into unforeseen trouble in its debut. I will also likely add a special reaction trigger for it as well.... later.