-
Notifications
You must be signed in to change notification settings - Fork 12
More chat patterns (timestamp and log), take 2 #49
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
base: main
Are you sure you want to change the base?
More chat patterns (timestamp and log), take 2 #49
Conversation
…em too Also fixes a Fabric-only bug where global messages get recorded only when someone doesn't have a prefix set up
3b18c3b to
7198eb7
Compare
|
The build seems to be failing. |
|
On one hand, it ended up with a bunch of issues due to writing this blindly without building. On another, it was not that bad? For some reason |
| "moreiotas.page.strings.string/chat/caster": "Adds the last message the caster sent to the stack as a string.", | ||
| "moreiotas.page.strings.string/chat/all": "Adds the last message anyone sent to the stack as a string.", | ||
| "moreiotas.page.strings.string/chat/timestamp/caster": "Returns the time in 20ths of a second since the world's beginning, to when the caster sent the last message. Though sometimes it just gives back a zero...", | ||
| "moreiotas.page.strings.string/chat/timestamp/all": "Similar to $(l:moreiotas:patterns/strings#moreiotas:string/chat/timestamp/caster)$(action)Sandglass' Reflection/$, but tells when anyone sent a message for all to hear. Oddly, returns a count under that too. But it's not like two people would speak at the same single instant, right?", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "moreiotas.page.strings.string/chat/timestamp/all": "Similar to $(l:moreiotas:patterns/strings#moreiotas:string/chat/timestamp/caster)$(action)Sandglass' Reflection/$, but tells when anyone sent a message for all to hear. Oddly, returns a count under that too. But it's not like two people would speak at the same single instant, right?", | |
| "moreiotas.page.strings.string/chat/timestamp/all": "Similar to $(l:patterns/strings#moreiotas:string/chat/timestamp/caster)$(action)Sandglass' Reflection/$, but tells when anyone sent a message for all to hear. Oddly, returns a count under that too. But it's not like two people would speak at the same single instant, right?", |
object-Object
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left several comments with specific changes that need to be made, as well as some questions and higher-level concerns.
| const val DEFAULT_MAX_CHAT_LOG = 341 // 3 within 1024 | ||
| const val MIN_MAX_CHAT_LOG: Int = 0 | ||
| const val MAX_MAX_CHAT_LOG: Int = 341 // TODO: take into account the stack size config... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a really weirdly specific default. What about something like 256 instead? Or 64 might be a more reasonably balanced value.
|
|
||
| override fun execute(args: List<Iota>, env: CastingEnvironment): List<Iota> { | ||
| if (!allChat) | ||
| return IXplatAbstractions.INSTANCE.lastMessageTimestamp(env.castingEntity as? Player).asActionResult |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this should either be on the same line as the if, or it should be wrapped in {} for clarity.
| override fun execute(args: List<Iota>, env: CastingEnvironment): List<Iota> { | ||
| if (!allChat) | ||
| return IXplatAbstractions.INSTANCE.lastMessageTimestamp(env.castingEntity as? Player).asActionResult | ||
| return listOf<Iota>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: is the type required here?
| { | ||
| "type": "hexcasting:pattern", | ||
| "op_id": "moreiotas:string/chat/timestamp/caster", | ||
| "anchor": "moreiotas:string/chat/timestamp/caster", | ||
| "input": "", | ||
| "output": "str", | ||
| "text": "moreiotas.page.strings.string/chat/timestamp/caster" | ||
| }, | ||
| { | ||
| "type": "hexcasting:pattern", | ||
| "op_id": "moreiotas:string/chat/timestamp/all", | ||
| "anchor": "moreiotas:string/chat/timestamp/all", | ||
| "input": "", | ||
| "output": "str", | ||
| "text": "moreiotas.page.strings.string/chat/timestamp/all" | ||
| }, | ||
| { | ||
| "type": "hexcasting:pattern", | ||
| "op_id": "moreiotas:string/chat/log/all", | ||
| "anchor": "moreiotas:string/chat/log/all", | ||
| "input": "", | ||
| "output": "str", | ||
| "text": "moreiotas.page.strings.string/chat/log/all" | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The input/output needs to be updated for all of these.
| "hexcasting.action.moreiotas:string/chat/all": "Listener's Reflection", | ||
| "hexcasting.action.moreiotas:string/chat/timestamp/caster": "Sandglass' Reflection", | ||
| "hexcasting.action.moreiotas:string/chat/timestamp/all": "Clocktower's Reflection", | ||
| "hexcasting.action.moreiotas:string/chat/log/all": "Stenographer's Reflection", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not a reflection if it pops values from the stack.
|
|
||
| if (!text.startsWith(prefix)) | ||
| // Just in case! People configure stuff in the weirdest ways | ||
| if (server.maxChatLog == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if it's less than 0?
| while (messageLog.isNotEmpty() && messageLog.size > server.maxChatLog) { | ||
| // Just in case somehow we end up putting two "at once" even though this is only place this is touched | ||
| messageLog.removeFirst() | ||
| } | ||
|
|
||
| messageLog.addLast(ChatEntry(text, timestamp, player.name.string)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If messageLog.size >= server.maxChatLog at the start, then this code results in messageLog containing server.maxChatLog + 1 entries. I think the loop condition should be >= instead of > to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing this on Discord, I think this pattern should be changed to pop nothing from the stack and push a list of lists, where each inner list is [message, username]. The list should contain all public chat messages that were sent in the most recent tick where at least one user sent a chat message.
For example:
- If no chat messages have been sent since the server started:
[] - If the most recent chat message was sent by
object_Object, and only one message was sent that tick:[["message contents", "object_Object"]] - If both
object_Objectandchloetaxsent a message 1 second ago:[["message 1", "object_Object"], ["message 2", "chloetax"]]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some comments to this and the Forge equivalent to clarify the control flow and what happens in each case? (eg. sender doesn't have a prefix set, sender has a prefix set but the message doesn't start with it, sender has a prefix and the message starts with it)
| if (prefix == null) { | ||
| lastMessages[player.uuid] = text | ||
| lastMessage = text | ||
| return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed?
|
Also, the build is still failing. Have you tested this locally at all? |
Now directing it to main, and added Forge!
Uh, I've not tested if it even builds, Gradle decided to be shit to me. So I need some help here, whee.. :'D