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

Dynamic ProgressComponent text color #1123

Merged
merged 6 commits into from
Jan 5, 2025

Conversation

DatL4g
Copy link
Contributor

@DatL4g DatL4g commented Jan 2, 2025

Applies a better text color depending on the bar color.

For example while doing comms the bars may be yellow and the text is hard to read because it's tinted white.
Now the text color is black when the bar is bright and white otherwise.

@LifeIsAParadox LifeIsAParadox added the reviews needed This PR needs reviews label Jan 2, 2025
@kevinthegreat1
Copy link
Collaborator

kevinthegreat1 commented Jan 2, 2025

Please test your features before submitting them lol. The description should always be white, and when drawing dark text, you should draw without shadow. Also, you can put bright in a boolean field so it doesn't get calculated every frame.

Screenshot 2025-01-02 at 14 40 04

@kevinthegreat1 kevinthegreat1 added changes requested This PR need changes and removed reviews needed This PR needs reviews labels Jan 2, 2025
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels Jan 2, 2025
@DatL4g
Copy link
Contributor Author

DatL4g commented Jan 2, 2025

Sorry I've never contributed to Minecraft mods.
I applied the changes you requested, one thing I wanted to add but not sure which values to use for calculation is applying dark text color only if the bar is shown to a certain point.

@kevinthegreat1
Copy link
Collaborator

Sorry I've never contributed to Minecraft mods. I applied the changes you requested, one thing I wanted to add but not sure which values to use for calculation is applying dark text color only if the bar is shown to a certain point.

No worries. In render, endOffsX is the width of the colored area. You can use TextRenderer#getWidth to get the width of a text.

Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

Hopefully last thing.

@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed reviews needed This PR needs reviews labels Jan 3, 2025
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels Jan 3, 2025
@DatL4g
Copy link
Contributor Author

DatL4g commented Jan 3, 2025

Hopefully last thing.

Hopefully, if I have to write one more Java line I'm gonna age 50 years :)

Anyway I think it looks good now, a hint from my testing (only comms, not farming or anything):

Only the beginning red color actually matches a bright text color, so it could be even more simplyfied by just changing to black when the bar reaches the end of the text, regardless of the color brightness.

However I think this solution is a lot better if you use this component in multiple places with different color values, or if you ever change the color by progress calculation.

Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Jan 3, 2025
@kevinthegreat1 kevinthegreat1 merged commit 15bce9f into SkyblockerMod:master Jan 5, 2025
1 check passed
@LifeIsAParadox LifeIsAParadox removed the merge me please Pull requests that are ready to merge label Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants