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

Runechat - stop bugging me for gods sake #14141

Merged

Conversation

AffectedArc07
Copy link
Member

@AffectedArc07 AffectedArc07 commented Aug 24, 2020

NOTE: This PR has been entirely re-written with new screenshots and a rewrite of the description

What Does This PR Do

image

This PR adds runechat. It is not the same as TGs implementation (specifically we dont have popups for radio or emotes), and thats becaus at 150 pop the server cant take it. Im not sacrificing performance for a feature as minor as this.

You will only see a runechat above someones head if you would actually see a message in chat. Examples include:

  • You wont see a whisper runechat if you cannot hear that person perfectly (you will see the asterisked out message though)
  • You wont see runechats from people speaking down radio channels you cant hear
  • You wont see runechats on people out of range
  • You will not see a message in a specified language if you do not have said language

If you are a crewmember, your message colour is picked from either your hair colour, your body colour if your species lets you set body colour, or your eye colour if you are a grey. Screenshots below
image
image
image
image

Language colouring takes precedence over anything else
image
image

Credit to @blinkdog for the HTML strip-out code used in parts of this. Everything else is ported from TG, with the exception of the radio icon colouring and species stuff colouring, which I did myself.

Again, I must state that this PR will not be merged if the performance is not up to par. Yes its a cool feature, but server performance is a top priority, and that is not changing

Why It's Good For The Game

1: People like this
2: I am fed up of people saying "AA WHEN ARE YOU PORTING RUNECHAT"

Changelog

🆑 AffectedArc07
add: Runechat
/:cl:

@ppi13
Copy link
Contributor

ppi13 commented Aug 24, 2020

This is how god intended ss13 to be played.

@TDSSS
Copy link
Contributor

TDSSS commented Aug 24, 2020

Is this a toggle? Please tell me I can just ignore this if it's horrible.

@datlo
Copy link
Contributor

datlo commented Aug 24, 2020

This definitely should be a toggle if it's added at all which I am not sure it should.

@ZomgPonies
Copy link
Contributor

ZomgPonies commented Aug 24, 2020

Probably want to keep colors strictly for languages like it is in textual chat for consistency reasons. You can use different font sizes for whispers/emotes (Fulp has a really clean runechat emote implementation). Don't forget stuff like loudspeaker, voice of god, etc.

tg also added a complex layering system for it so more recent messages automatically stack on top of someone else's older one.

@AffectedArc07
Copy link
Member Author

Yes, this is a toggle, dont worry.

@ZomgPonies this is a barebones PR for TMing only. If the heads and maints deciede its good, ill polish it and add all the other pizzazz in.

@AffectedArc07 AffectedArc07 added Feature This PR is a new addition to the game Literally Everybody Ded Were we ever alive? Test Merge Candidate Work In Progress This PR is work in progress, and unfinished WTF BYOND will never cease to amaze and/or disappoint us labels Aug 24, 2020
@Aurorablade
Copy link
Contributor

Its horrible and a jumbled mess by the pictures and I do not feel we need this.

@AffectedArc07 AffectedArc07 added the Do Not Merge This PR must not be merged or closed label Aug 24, 2020
@ZomgPonies
Copy link
Contributor

It's nowhere as bad as the pictures -- Spend a round observing on any other popular server to get an actually accurate feel for how busy it is on screen.

@Aurorablade
Copy link
Contributor

Runechat, ultima online chat call it what you will I have been around many games with this type of thing in a isometric envrioment.it sucks. It's more tolerable in a 3d environment

@AffectedArc07
Copy link
Member Author

image
image
image

image
@ZomgPonies Considering we have anywhere from 2-4x the population of TG, not to mention more RP and a smaller station, I can see this going horribly wrong.

TG doesnt have prisoner processing. This is their processing area on meta
image
If the grime on the floor doesnt give it away, its never used because people are brigged into the cells directly

Here is an average sized runechat message being simulated over our prisoner processing area, fully loaded (which can and has happened more often that not)
image

You're telling me this is readable?

@OctusGit
Copy link
Contributor

OctusGit commented Aug 24, 2020

LETS FUCKING GOOOOOOOOOOOOOOOOO
also, you people do realize you can TURN IT OFF if you don't wanna see it right? like cmon, this argument isn't logical because there is an option to DISABLE it. everyone acting like there being forced to play with it, as a person whos wanted this for a long time I DESPISE having to read the textbox and move my eyes to the game just to see it's flooded with chaos and other shit, this is a quality of change everyone should be on board about because not only is it UI friendly, it also appeases both sides, the people who DO and DONT want it. And @AffectedArc07, the poison well tactic, and garbage example from Reddit are pretty funny and all but you really trying to make it seem like its a bad PR when almost all other servers have this in-game, there's a reason why it's so popular. Oh and if you really need to find a specific part of the text you missed, the text box is STILL there.

@Carthusia
Copy link

I look forward to this, so long as its a toggle, the people who would love this are happy, and the people who dont, are happy.

I like the idea of being able to focus on one screen rather than having to teach my brain to seperate my eyes in seperate directions like im an inguana to see both the main action screen, and a text screen like a 1980's Text RPG

@AffectedArc07
Copy link
Member Author

Putting this in a comment to make it more noticable from the PR description. The point of this testmerge is not just a "do we want this", theres a large performance factor at play here too. In particular, 2 things will be measured.

1: General profile of the server (Whats taking longest to execute). The messages heavily rely on calls to animate(), which is one of the most expensive operations in the entire game
2: GC rate of messages (In technical terms, when these things delete do they still have references being held)

This is in no way a final product, nor do I have intention to actually get this fully merged in, given what I know about BYOND and how badly this system will handle at 120 population.

@AffectedArc07
Copy link
Member Author

LETS FUCKING GOOOOOOOOOOOOOOOOO
also, you people do realize you can TURN IT OFF if you don't wanna see it right? like cmon, this argument isn't logical because there is an option to DISABLE it. everyone acting like there being forced to play with it, as a person whos wanted this for a long time I DESPISE having to read the textbox and move my eyes to the game just to see it's flooded with chaos and other shit, this is a quality of change everyone should be on board about because not only is it UI friendly, it also appeases both sides, the people who DO and DONT want it. And @AffectedArc07, the poison well tactic, and garbage example from Reddit are pretty funny and all but you really trying to make it seem like its a bad PR when almost all other servers have this in-game, there's a reason why it's so popular. Oh and if you really need to find a specific part of the text you missed, the text box is STILL there.

@Tokorizo lets break this down, piece by piece.

also, you people do realize you can TURN IT OFF if you don't wanna see it right? like cmon, this argument isn't logical because there is an option to DISABLE it. everyone acting like there being forced to play with it

Yes it can be disabled, but you shouldnt be expected to have to modify your preferences every time you move in and out of a highly populated area, thats bad game design. No one is acting like they are being "forced to play with it". I mentioned in a comment yesterday that it will be a toggle, and have since updated the PR description to make it even more obvious that its a toggle. No one is being forced here.

And @AffectedArc07, the poison well tactic, and garbage example from Reddit are pretty funny and all

This screenshot has been posted around multiple places, and is the prime example of why runechat does not work in crowded environments. I also dont agree with calling this "poisoning the well". The entire point of this PR, is to show it to the server population, with examples of why it wont work here. The whole point is for it to be testmerged for player satisfaction, and for performance (Keep in mind, this puts substantial load on SStimer, and relies a lot on animate() calls

almost all other servers have this in-game, there's a reason why it's so popular.

I said this before, and I will say it again. It works on servers like TG because they dont have an RP level. The processing situation literally cannot exist on TG, because this is their processing area, on DeltaStation keep in mind, their biggest map.
image
TG doesnt have the same situations we do. A lot of HRP servers still dont have this because of the issues it creates (Take Bay12 and AuroraStation for example). It does not work in high RP environments where you will have multiple people in a small area.

@Krysonism
Copy link

Neither the screenshot in the OP or the mock-up is representative of how runechat actually looks in-game.

I am pretty sure the screenshot in OP was taken while the timer subsystem broke and the messages were not clearing properly.

In my opinion runechat facilities RP by making it easier notice when someone is trying to talk to you.

@procdrone
Copy link
Contributor

I mean Affected, staff and you did this to yourselves by going super hush hush on why you wouldn't add it in the first place, refusing to give any explanation.

Other than that, a very welcome addition.

Also don't say we have TONS more roleplay. server been edging into LRP side for quite some time now. but, thats not a place to discuss state of the server.

Additionally, you show TG shuttle as an example why bad. That shuttle is tiny. and its easier to notice someone saying something by this than constantly FLOWING textbox.

Also sure we have 120 people. 120 throughout the whole station. rarely people pile up in one place except shuttle.

Sure, there might be downsides but there is a reason why almost everyone adopted this.

@Carthusia
Copy link

Something I have to note as well after last nights shuttle, but the picture of chat all over the place looking really bad, is more or less the same as what we have now in high populated areas where it's nearly impossible to read what's being said to you during lots of action.

I find on a lot of shuttles I cant really even talk anymore due to the high amount of red text actions or other chat being done.

Though other than extreme high pop areas like that, I feel like this would be more or less fine to witness, I look forward to its test to see for myself though.

@Dumbdumn5
Copy link

Personally? I think runechat is cool as fuck. I don't know the technical bit about animate calls though. Do you have any way you could see how much it'd impact CPU load for those of us not into the wizard speak but who think it's a neat idea regardless of the potential visual clutter? Can have it default to off and just let those who like it turn it on for that particular problem.

@OctusGit
Copy link
Contributor

@AffectedArc07
Allow me to return the favor, and thanks for being so calm and respectful in your rebuttal.

Yes it can be disabled, but you shouldnt be expected to have to modify your preferences every time you move in and out of a highly populated area, thats bad game design. No one is acting like they are being "forced to play with it". I mentioned in a comment yesterday that it will be a toggle, and have since updated the PR description to make it even more obvious that its a toggle. No one is being forced here.

Firstly I'm sure a majority of people will always keep rune chat on, even if the situation is chaotic. I doubt every time the situation is chaotic people will turn it off and that shouldn't be the mentality for all players. Its MUCH better than attempting to read the chatbox with emotes, actions, comms and other minor things that flood the chat and make it INCREDIBLY difficult to read what is happening, it's so constant that people will usually miss people talking to them. This problem is NOT just "Runechat" its the chaotic nature of SS13 and its WORST when I'm forced to try and have a conversation with someone on the shuttle only for what I've said previously to be spamming the chat. Its WORST gameplay design to have your eyes moving back and forth between the chatbox and gameplay window while also having said chatbox moving at the speed of light due to spam from other sources. I repeat, a quality of life change that keeps the important stuff on one screen, dialogue and gameplay. So no, I disagree that its "bad" gameplay design compared to what we currently have.

This screenshot has been posted around multiple places, and is the prime example of why runechat does not work in crowded environments. I also dont agree with calling this "poisoning the well". The entire point of this PR, is to show it to the server population, with examples of why it wont work here. The whole point is for it to be testmerged for player satisfaction, and for performance (Keep in mind, this puts substantial load on SStimer, and relies a lot on animate() calls

I don't think this is the fault of runechat for being such a chaotic scene. Imagine if I was trying to keep track of this in the CHATBOX, crowded environments are ALWAYS chaotic, I get a headache every time there are 6-sec officers in processing and they're spamming a cup of water in the chatbox making me miss things. Also what @Krysonism said...

Neither the screenshot in the OP or the mock-up is representative of how runechat actually looks in-game.

I am pretty sure the screenshot in OP was taken while the timer subsystem broke and the messages were not clearing properly.

I more inclined to believe that this was a bug with the system that resulted in messages not clearing because that would mean ALL players had to have said something at the same time which really isn't the common case for runechat. I occasionally play fulp (very similar to /tg/ but better) and I've never had a headache from reading runechat text, and ive never seen the example you've used actually happen. The picture you use is a bad example not only because I'm certain this picture is a bug to show how "bad" this feature is, but you also leave out any interesting and appealing things in the first page, and yes I understand YOU don't like it, but this goes back to what I was saying about poisoning the well. Also the "it won't work here" isn't really fair to say, its a common PR that gets used on server pops higher than ours (CM, Fulp, Etc) so if this really didn't work on other servers I'm a bit surprised why they haven't just removed it... maybe because its a popular PR that many people agree is a major quality of life change. Also, I understand the point behind optimization and that's a fair and valid point against rune chat no question about it. But I'm more inclined to believe we'll be fine, if CM and Fulp which sometimes have over 120+ can handle it, I'm not sure why we can't. But again well see what happens.

I said this before, and I will say it again. It works on servers like TG because they don't have an RP level. The processing situation literally cannot exist on TG, because this is their processing area, on DeltaStation keep in mind, their biggest map.
image
TG doesnt have the same situations we do. A lot of HRP servers still dont have this because of the issues it creates (Take Bay12 and AuroraStation for example). It does not work in high RP environments where you will have multiple people in a small area.

I feel like a more fair comparison for RP levels and an example for this PR would be fulpstation, their MRP, have a similar player count than us, and actually have a lot of the features we want. I also don't really understand what the processing example is, sorry I don't seem to understand. Also, I think the reason why Bay and Aurora don't have this feature is that at most their player count is 50, so they really don't have chaotic moments. And what do you mean "multiple people in a small area" are we talking three people in the bridge? 6 people in processing? 50 people on the shuttle? I think it can work with multiple people in a small area, that's the issue with having such a high player count, it's going to be CHAOTIC, rune chat or not.

But again, I respectfully disagree with most of your points but I do agree on a possible performance issue, I'm sure there is something we can do about it and again, looking forward to this PR.

@shazbot194
Copy link
Contributor

shazbot194 commented Aug 25, 2020

image

I don't think this is at all readable, and this is only roughly 14 people talking, let along 20, 30, or 100. This would be awful for any sizable group of people more than 3 people.

@OctusGit
Copy link
Contributor

image

I don't think this is at all readable, and this is only roughly 14 people talking, let along 20, 30, or 100. This would be awful for any sizable group of people more than 3 people.

This picture is more than likely a bug and doesn't show a good example of what it would look like in-game, this is a poor extreme compared to how it actually would look.

@Tarhalindur
Copy link

Test merge seems to indicate that Rune is broken with tcomms, it says "says" twice.

@Tarhalindur
Copy link

Also, there's another fact about runechat. What does this actually add?

It doesn't let you ignore normal chat, because radio still appears there so you need to look there anyway. So I don't get what it adds beyond random graphical fluff that we don't really need and that is potentially detrimental to the server.

@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Sep 6, 2020
@AffectedArc07
Copy link
Member Author

Holopads have been fixed.

Before:
image

After:
image

Note the text wont always be grey, these two test characters just had the default hair colour

@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Sep 6, 2020
@AffectedArc07
Copy link
Member Author

/datum/chatmessage
- qdel() Count: 64215
- Total Destroy() Cost: 2184ms
- Average Destroy() Cost: 0.034ms

^ Latest TM qdel result

                                                       Profile results (total time)
Proc Name                                                                                   Self CPU    Total CPU    Real Time     Overtime        Calls
---------------------------------------------------------------------------------------    ---------    ---------    ---------    ---------    ---------
/datum/controller/subsystem/runechat/fire                                                      0.873        6.533        6.550        0.000       147555
/mob/living/carbon/human/get_runechat_color                                                    0.062        0.167        0.174        0.008        57507
/datum/species/proc/get_species_runechat_color                                                 0.060        0.100        0.104        0.009        56460
/client/verb/toggle_runechat                                                                   0.001        0.007        0.007        0.000           48
/atom/proc/get_runechat_color                                                                  0.004        0.004        0.004        0.001         6448
/datum/species/grey/get_species_runechat_color                                                 0.001        0.002        0.003        0.000         1047
/datum/controller/subsystem/runechat/stat_entry                                                0.000        0.003        0.003        0.000          190
/datum/controller/subsystem/runechat/fire                                                      0.873        6.533        6.550        0.000       147555
/mob/living/carbon/human/get_runechat_color                                                    0.062        0.167        0.174        0.008        57507
/datum/species/proc/get_species_runechat_color                                                 0.060        0.100        0.104        0.009        56460
/datum/chatmessage/proc/generate_image                                                         5.119        8.973    21589.590        0.408        63955
/datum/chatmessage/New                                                                         0.146        5.792        5.785        0.020        63955
/datum/chatmessage/proc/end_of_life                                                            1.996        2.419        2.428        0.000        63901
/datum/chatmessage/Destroy                                                                     1.316        2.067        2.070        0.000        63904
/datum/chatmessage/proc/sanitize_color                                                         0.521        2.013        2.019        0.090        63955
/datum/chatmessage/proc/enter_subsystem                                                        0.607        0.614        0.634        0.009       138934
/datum/chatmessage/proc/leave_subsystem                                                        0.194        0.196        0.198        0.000        63904
/datum/chatmessage/proc/colorize_string                                                        0.006        0.023        0.023        0.002          507

^ Latest TM profile

I think this should be fine, though ill let other maints make the decision

@Spacemanspark
Copy link
Contributor

Bad, start from scrap and do it all over again

@AffectedArc07
Copy link
Member Author

Ghosts not seeing runechats has now been deemed intentional as a ghost could enable seeing all chat messages in the world, and potentially bog down the server from the amount of messages.

This PR will stay in the draft state until #13936 is merged

@github-actions github-actions bot added the Merge Conflict This PR is merge conflicted label Sep 12, 2020
@github-actions github-actions bot removed the Merge Conflict This PR is merge conflicted label Sep 13, 2020
@AffectedArc07
Copy link
Member Author

SQL Query to enable runechat for all pre-existing players

UPDATE `player` SET `toggles_2` = `toggles_2` + 64

@AffectedArc07 AffectedArc07 marked this pull request as ready for review October 9, 2020 19:24
@AffectedArc07 AffectedArc07 removed the Do Not Merge This PR must not be merged or closed label Oct 9, 2020
@@ -88,3 +88,7 @@
H.adjustFireLoss(1)
return TRUE
return ..()

/datum/species/grey/get_species_runechat_color(mob/living/carbon/human/H)
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to account for drask and kidan too.

Copy link
Member Author

Choose a reason for hiding this comment

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

image
image

@github-actions github-actions bot added Merge Conflict This PR is merge conflicted and removed Merge Conflict This PR is merge conflicted labels Oct 15, 2020
@AffectedArc07 AffectedArc07 removed the Merge Conflict This PR is merge conflicted label Oct 17, 2020
@variableundefined variableundefined merged commit ca29f53 into ParadiseSS13:master Oct 26, 2020
@Spacemanspark
Copy link
Contributor

Yo arc when we getting that iPhone imessage chat system

@AffectedArc07
Copy link
Member Author

@Spacemanspark
image

@Spacemanspark
Copy link
Contributor

Hot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature This PR is a new addition to the game
Projects
None yet
Development

Successfully merging this pull request may close these issues.