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

Introducing custom blips: as easy as a custom showname! #780

Closed
wants to merge 16 commits into from

Conversation

Lernos
Copy link
Contributor

@Lernos Lernos commented Jun 5, 2022

Requires modifications from server owners for their servers to include 'custom_blips' in their "FL" packet and a new "validation monstrosity" to include a new string called blipname which must be added to the IC message args processed by the server. For backwards compatibility I suggest setting blipname as a server-side args variable to an empty string "" by default, since older clients will send nothing. In that case, the client will read the char.ini file to determine the correct blips/gender instead of looking it up in the new IC message packet.
P.S. Obviously themes need to be edited too, just add an ao2_custom_blips box somewhere near the ao2_ic_chat_name as needed.

@Lernos
Copy link
Contributor Author

Lernos commented Jun 5, 2022

I know, clang-tidy, I don't love you either.

@stonedDiscord
Copy link
Member

../general/sfx-guitar2

@in1tiate
Copy link
Member

in1tiate commented Jul 7, 2022

This does not need to be part of the IC validation hell string, and as a whole blips do not need to be networked. Voting for closure.

@Lernos
Copy link
Contributor Author

Lernos commented Jul 7, 2022

Why do they not need to be networked though? We allow to play custom SFX on any message, but not blips? How about the fact that canonically, Godot can imitate voices of other characters, Ted Tonate can speak either as a "ddmale" or "tonate" typing on his keyboard, Robin changes from male to female voice after her transformation, if you want to have an invisible narrator ("Anon" type of character), you'd have to make one for each blipvoice? These things are all important to players I know. Changing a character's blips to any other blips on the fly seems like a very nice customization option.

@Salanto
Copy link
Contributor

Salanto commented Jul 14, 2022

This is double edged.
It has terrible abuse potential, but at the same time is very flexible frame_sfx is already being used to earrape everyone around them and I doubt this would fare much better.

@Lernos
Copy link
Contributor Author

Lernos commented Jul 14, 2022

Well, you can't just turn frame_sfx off from my understanding. While here, a checkbox to stop displaying custom blips and fall back to the default char.ini ones could be easily coded in, similar to custom shownames. And yes, the very first portion of blip sounds coming in could be earrapey, but you could do things about it. As a player, you can turn down your blips volume or set your blips frequency in the settings to something else, then OBJECTION-interrupt the person speaking until they are dealt with by a mod. As a coder, you could additionally restrict which blips can be played by adding a function that only searches inside the blips folder instead of the whole sounds/general folder, if you so desire. Then it falls upon the custom content managing team to prevent abuse-worthy blips of entering that folder. It's immensely easier to fix this than frame_sfx, yet frame_sfx are already in the client. In any case, I'd prefer to see more flexible features added in with additional anti-abuse measures rather than them being shot down immediately even though we already allow something worse.

@in1tiate
Copy link
Member

in1tiate commented Jul 15, 2022

Why do they not need to be networked though? We allow to play custom SFX on any message, but not blips? How about the fact that canonically, Godot can imitate voices of other characters, Ted Tonate can speak either as a "ddmale" or "tonate" typing on his keyboard, Robin changes from male to female voice after her transformation, if you want to have an invisible narrator ("Anon" type of character), you'd have to make one for each blipvoice? These things are all important to players I know. Changing a character's blips to any other blips on the fly seems like a very nice customization option.

I hadn't considered characters with multiple blip types, which is a feature the unofficial 2.5 client had and which we currently still lack. With that in mind I'm in favor of this change but I'd prefer it to be char.ini-configurable rather than add to the Courtroom UI and deal with all the headache involved in that.

Copy link
Member

@in1tiate in1tiate left a comment

Choose a reason for hiding this comment

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

See above; please avoid adding clutter to the main UI

@TrickyLeifa TrickyLeifa added enhancement Request for functionality not present engine/viewport Issues related to viewport positioning and layering engine/audio Issues related to music and sound effect playback labels Jul 22, 2022
@TrickyLeifa
Copy link
Contributor

I'd recommend linking a server pull-request that introduces the network changes so that it's easier to test the changes; and vice versa.

Copy link
Member

@oldmud0 oldmud0 left a comment

Choose a reason for hiding this comment

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

Looks fine but seems abusable - e.g. set blip name to ../music/zettaslow.

Also looking for the serverside PR for this.

Also put that profile picture away.

src/courtroom.cpp Outdated Show resolved Hide resolved
@@ -177,6 +177,11 @@ Courtroom::Courtroom(AOApplication *p_ao_app) : QMainWindow()
ui_ic_chat_name->setPlaceholderText(tr("Showname"));
ui_ic_chat_name->setText(p_ao_app->get_default_showname());
ui_ic_chat_name->setObjectName("ui_ic_chat_name");

ui_custom_blips = new QLineEdit(this);
Copy link
Member

Choose a reason for hiding this comment

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

Oh yeah also this definitely should be a dropdown. It's not ideal to just throw the player into the darkness and make them scroll through the files themselves.

@Salanto
Copy link
Contributor

Salanto commented Aug 1, 2022

By the by, @Lernos. You could join the Discord to make communication a bit easier.

@Crystalwarrior
Copy link
Contributor

Crystalwarrior commented Aug 1, 2022

Looks fine but seems abusable - e.g. set blip name to ../music/zettaslow.

Also looking for the serverside PR for this.

Also put that profile picture away.

Well servers should sanitize it in that case lmao
Also same, serverside PR would be nice

@Lernos
Copy link
Contributor Author

Lernos commented Aug 2, 2022

Also put that profile picture away.

Sorry xD

By the by, @Lernos. You could join the Discord to make communication a bit easier.

Oh, I actually am. I was just dealing with stuff IRL. Not sure it would make communications that much easier, but whatever's more convenient.

Also same, serverside PR would be nice

There you go. I don't work with akashi, but I'm pretty sure it can be figured out. There should be a similar mechanism there somewhere, right?
AttorneyOnline/tsuserver3#198

@Salanto
Copy link
Contributor

Salanto commented Aug 2, 2022

I'll handle Akashi. You might want to resolve those merge conflicts tho.

@stonedDiscord
Copy link
Member

stonedDiscord commented Sep 2, 2022

Oh yeah also this definitely should be a dropdown. It's not ideal to just throw the player into the darkness and make them scroll through the files themselves.

but which sounds should the dropdown contain?

  1. all entries from a blips.ini file
  2. every .opus file in base\sounds\blips
  3. every file in base\sounds\general that starts with sfx-blip
  4. every file from every subfolder in base\sounds\blips
  5. all of the above

if we go with 5, what to do about the list getting too long?
my vote is on 2.

@oldmud0
Copy link
Member

oldmud0 commented Sep 7, 2022

I would go with 2 although it wouldn't be much additional effort to go with 4.

@in1tiate
Copy link
Member

in1tiate commented May 9, 2024

I plan on pulling the network changes from this pull request into the code I'm writing for #952 unless there are any objections. I will not be including the UI changes, as they are outside the scope of that issue.

@Salanto
Copy link
Contributor

Salanto commented May 16, 2024

Stale PR + Implemented in a different PR
Thanks a bunch for the ground work, Lernos!

@Salanto Salanto closed this May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engine/audio Issues related to music and sound effect playback engine/viewport Issues related to viewport positioning and layering enhancement Request for functionality not present
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants