More chat patterns (timestamp and log), take 2#49
More chat patterns (timestamp and log), take 2#49SaphireLattice wants to merge 5 commits intoFallingColors:mainfrom
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.
| "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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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 |
|
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