bergerhealer / TrainCarts

Minecarts redefined
MIT License
206 stars 64 forks source link

feat: add title action sign #480

Closed layertwo closed 1 year ago

layertwo commented 1 year ago

Overview

Minecraft has the sendTitle feature, where a title can be display on the player's screen. I've added a new sign to support this feature. Configuration values for title fade in, stay, fade out are all optional values, but can be provided on the title line for additional configuration.

Example syntax:

[train]
title 10 70 10
foo
bar

Follow up review from #479

bbayu123 commented 1 year ago

Same concern as before: The regular send message method parses SignLink variable names as well, so I wonder if it should be included in titles as well. https://github.com/bergerhealer/TrainCarts/blob/28466a595d2078124664a159638744ed20b6642f/src/main/java/com/bergerkiller/bukkit/tc/TrainCarts.java#L403-L419

I would believe that no one would be insane enough to try doing this on a regular sign. However, TCC node signs have (theoretically) infinite text length on each line, so it's possible to inject as many SignLink variables as needed.

For example, in a metro scenario, you can put This train is %bananaStationN% going to %bananaStationD% which would show the name and destination for the train that tripped the bananaStation trigger sign. I can see a benefit to also introduce this into the title sign, but I just don't know how necessary it would be.

bergerkiller commented 1 year ago

More important now that signlink can be used to proxy PlaceholderAPI placeholders as well, so theres even more use for it. That said that syntax needs an overhaul.

layertwo commented 1 year ago

Is there anything else you need me to do on this PR? Don't want to introduce feature creep.

bergerkiller commented 1 year ago

You need to add one more thing.

Sending a title was added to the Bukkit API in Minecraft version 1.9, however, only since server version 1.11 are there fade-in/out parameters for that method. So where you register the SignAction, can you add a Common.evaluateMCVersion(">=", "1.11") check? Cleaner to not even enable the sign if it cannot be used on the server.

I dont know if sending a title with fade is at all possible 1.9-1.10.2, maybe bukkit lacked an API. But theres no need to add an api for this to bkcommonlib as I doubt that many people will use it pre-1.11 anyway.

The isType() check isn't needed in build() and can be omitted. Thats already handled by match() which runs before build() to identify the sign anyway.

No other comments except for probably better support for placeholderapi/signlink in the future, but thats more up to me to fix (Traincarts.getMessage() needs to be improved)

bergerkiller commented 1 year ago

Technically you could for 1.9 - 1.10.2 only call the showTitle() api without fade params I guess. Might add that tweak myself

bergerkiller commented 1 year ago

Any update?

layertwo commented 1 year ago

Apologies, was dealing with a personal emergency. Will get back to this soon.

bergerkiller commented 1 year ago

No problem, take your time :)

layertwo commented 1 year ago

Rebased and pushed changes.