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

[1.21] Overhaul tps command to be more colorful and organized #1422

Merged
merged 10 commits into from
Aug 31, 2024

Conversation

ApexModder
Copy link
Contributor

@ApexModder ApexModder commented Aug 5, 2024

The /neoforge tps <dim> command output has been overhauled to be much more colorful, making it easier to read and understand at a glance.

Both the TPS and Tick Time components now have a “hover event” to better explain the values. Additionally, the mean TPS fades from green to red, making it quicker and easier to identify laggy dimensions.

Before

image

After

java_V1wncOEK0l

@ApexModder ApexModder added enhancement New (or improvement to existing) feature or request cleanup Change that isn't an enhancement or a bug fix 1.21 Targeted at Minecraft 1.21 labels Aug 5, 2024
@neoforged-pr-publishing
Copy link

neoforged-pr-publishing bot commented Aug 5, 2024

  • Publish PR to GitHub Packages

Last commit published: c2f464d7f7659ebf131a2fc30ad813fccd74685b.

PR Publishing

The artifacts published by this PR:

Repository Declaration

In order to use the artifacts published by the PR, add the following repository to your buildscript:

repositories {
    maven {
        name 'Maven for PR #1422' // https://github.com/neoforged/NeoForge/pull/1422
        url 'https://prmaven.neoforged.net/NeoForge/pr1422'
        content {
            includeModule('net.neoforged', 'neoforge')
            includeModule('net.neoforged', 'testframework')
        }
    }
}

MDK installation

In order to setup a MDK using the latest PR version, run the following commands in a terminal.
The script works on both *nix and Windows as long as you have the JDK bin folder on the path.
The script will clone the MDK in a folder named NeoForge-pr1422.
On Powershell you will need to remove the -L flag from the curl invocation.

mkdir NeoForge-pr1422
cd NeoForge-pr1422
curl -L https://prmaven.neoforged.net/NeoForge/pr1422/net/neoforged/neoforge/21.1.42-pr-1422-pr-tps-command/mdk-pr1422.zip -o mdk.zip
jar xf mdk.zip
rm mdk.zip || del mdk.zip

To test a production environment, you can download the installer from here.

@ApexModder ApexModder removed the cleanup Change that isn't an enhancement or a bug fix label Aug 5, 2024
@HenryLoenwind
Copy link
Contributor

HenryLoenwind commented Aug 5, 2024

Please don't drop the "ms" unit.

Please revert those commas back to periods or use lowercase after them. ", Mean" is just mean ;)

Also, I would suggest using ResourceLocation.toLanguageKey() instead of the raw RL (and putting the raw RL into the hover text). That way, it will translate automatically if we ever manage to put those language keys in for vanilla dimensions.

The "Dimension:" prefix seems unnecessary to me. I would drop it to avoid line breaking.

@ApexModder
Copy link
Contributor Author

ApexModder commented Aug 6, 2024

Added back the ms unit; I didn’t actually intend on removing it.

Dropped the dimension prefix and updated to auto-translate if a matching translation key exists (defaults to the raw registry name if missing): dimension.<namespace>.<path>

I have included translations for the vanilla dimensions, but modders will have to include their own.

The raw dimension registry name has also been added as a hover tooltip when hovering over the dimension component.

Below is an example of these changes. Note how the Nether is minecraft:the_nether; this is because no dimension.minecraft.the_nether translation exists.

Example

image

@ApexModder
Copy link
Contributor Author

Locked behind #1428, Will need updating if that PR goes through

@neoforged-automation
Copy link

@ApexModder, this pull request has conflicts, please resolve them for this PR to move forward.

@Matyrobbrt
Copy link
Member

I think it's better if the colours are part of the translation and use vanilla's weird character instead of being hard-coded, as it would be more customizable, especially for people that have a hard time with the built in colours.

@ApexModder
Copy link
Contributor Author

I think it's better if the colours are part of the translation and use vanilla's weird character instead of being hard-coded, as it would be more customizable, especially for people that have a hard time with the built in colours.

While this would work for the coloring, how would I go about applying the hover tooltips that way?
afaik it is not possible to apply hover effects other than code.
But I do agree having the colors being configurable in some way would be better (even if its just the translation defining the colors).

@HenryLoenwind
Copy link
Contributor

Just a side note: The json-array format probably can do hover, but it should not be used as it falls apart on Crowdin. The SGML-format could be extended to support hover and even parameterised hovers; I just never thought about this use case.

@Matyrobbrt
Copy link
Member

Matyrobbrt commented Aug 23, 2024

afaik it is not possible to apply hover effects other than code.

That's correct, that much can stay code, but the colouring should still be part of the lang entry.

Copy link
Member

@XFactHD XFactHD left a comment

Choose a reason for hiding this comment

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

One detail I'm not sure about, otherwise the code looks good to me

@Matyrobbrt Matyrobbrt changed the title [1.21] Overhaul output of tps command to be more colorful [1.21] Overhaul output of tps command to be more colorful and organized Aug 25, 2024
Copy link
Member

@sciwhiz12 sciwhiz12 left a comment

Choose a reason for hiding this comment

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

Two small comments remaining on my end. Otherwise, it looks fine.

Copy link
Member

@sciwhiz12 sciwhiz12 left a comment

Choose a reason for hiding this comment

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

Whoops, clicked the wrong button.

@sciwhiz12 sciwhiz12 requested a review from XFactHD August 25, 2024 14:04
sciwhiz12
sciwhiz12 previously approved these changes Aug 25, 2024
Copy link
Member

@sciwhiz12 sciwhiz12 left a comment

Choose a reason for hiding this comment

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

Looks okay to me, and it works. Waiting on @XFactHD to rereview.

Copy link
Member

@XFactHD XFactHD left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@sciwhiz12 sciwhiz12 changed the title [1.21] Overhaul output of tps command to be more colorful and organized [1.21] Overhaul tps command to be more colorful and organized Aug 31, 2024
@sciwhiz12 sciwhiz12 merged commit 98440b8 into neoforged:1.21.x Aug 31, 2024
6 checks passed
@neoforged-releases
Copy link

🚀 This PR has been released as NeoForge version 21.1.32.

@ApexModder ApexModder deleted the pr/tps-command branch September 16, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.21 Targeted at Minecraft 1.21 enhancement New (or improvement to existing) feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants