bergerhealer / TrainCarts

Minecarts redefined
MIT License
205 stars 63 forks source link

feat: add title action for announce sign #479

Closed layertwo closed 1 year ago

layertwo commented 1 year ago

Overview

The Announce sign currently only announce to a player's chat. Minecraft also has the sendTitle feature, where a title can be display on the player's screen. To not ruin backwards compatibility, I've created a method for determine if the announce is default, or if it is to use the title method.

A user can provide this option by adding an additional string "title" to the second line of a sign. Example

[train]
announce title
foo
bar

Notable changes

Strings are separate messages

Each message line on a sign is now sent as a separate message to players, rather than concatenating the strings together and sending that as a single message.

The current method concatenates all strings and sends them to the chat as a single message. This ends up being an issue because separate lines are merged into a single line and make it hard to read. Example: using "foo" on line 3 and "bar" on line 4 ends up sending to the player chat "foobar" rather than "foo bar".

AnnounceMessage

Created a private AnnounceMessage class that handles both title and subtitle (need multiple messages for sendTitle), and can be applied for sending a message to the player's chat as well.

This class provides a clean method for both sending to chat and sending title and subtitle to a player, and was the best path forward I considered for deconflicting having to spend cycles generating the message per player (instead it can be calculated ahead of time and only recalled when it needs to be sent).

bbayu123 commented 1 year ago

Three issuse that I can see here

  1. This change fundamentally breaks all existing announce signs that have more than 2 lines of messages, thus you are throwing out backwards compatibility.
  2. Since you are parsing for aliases separately for lines 2 and 3, aliases that are long enough to span both lines will not get parsed properly. Again throws out backwards compatibility
  3. The announce sign also parses SignLink variables in a message when they get sent, but your new titles don't do that.

If the logic is sufficiently different from the existing announce sign, it's better to create a new sign rather than adding functionality to an existing sign.

layertwo commented 1 year ago

Good call outs!

I definitely went back and forth when designing this trying to figure out how to keep the existing behavior and also be ablee to use sendTitle. The major reason for the AnnounceMessage class is that I didn't want to calculate the message during send time, and only try to calculate it once outside of the sending logic.

For 1: I think this could be solvable with an additional property in AnnounceMessage, and then send each new line as a message. The major change is here is that each line is sent separately and not as a single message. This is probably the larger discussion to be had.

For 2: This is totally fair. All depends on what the intended behavior of this sign is and if this change is acceptable.

Thanks for taking a look, more than happy to iterate on this and provide something that we all agree on.

bbayu123 commented 1 year ago

One last thing that I can think of: Since you currently don't allow for specifying a fade-in, stay, and fade-out time, so someone will eventually ask for it. What are the possible ways of extending this - that you can think of - so that the 3 values can be provided?

layertwo commented 1 year ago

Hmmm... good question. I think there's enough space to add this to line 2 with something like 20/70/10, but that'll probably end up feeling cramped.

bergerkiller commented 1 year ago

just a thought, why isn't it possible to add a new sign action completely called "showtitle" which does all the title handling? I kind of also want this because of build permission rules and stuff. But your solution is fine too.

Don't know about the fade-in time and stuff, that's difficult to specify. Would've been easier if it wasn't constrained to what can fit on a sign. But if the unit is in ticks, I think its fine to allow something like "showtitle 5 40 5" and stuff. That's not too long

bbayu123 commented 1 year ago

That's what I said as well

If the logic is sufficiently different from the existing announce sign, it's better to create a new sign rather than adding functionality to an existing sign.

NobodyBestFriend commented 1 year ago

Just a thought, but I think it would make sense for this to be a separate sign. You could also add support for subtitles and actionbars, similar to the /title command. Line 2 could be "title" (shorter than showtitle but still gets the point across) followed by "t" for title, "s" for subtitle, and "a" for actionbar, then the three number format for fade and stay.

layertwo commented 1 year ago

I think the consensus here is that a separate title sign is the best path forward. That works for me.

@NobodyBestFriend can you post the sign syntax like how I have in the overview of this PR? I'm not sure I exactly follow what you're thinking.

I'll close this PR and open a new one for a title sign.