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

Add wear bar color API #13328

Merged
merged 20 commits into from Feb 2, 2024
Merged

Add wear bar color API #13328

merged 20 commits into from Feb 2, 2024

Conversation

techno-sam
Copy link
Contributor

@techno-sam techno-sam commented Mar 15, 2023

Adds an api for modders to change the appearance of the wear bar on their tools.

Currently, the tool damage bar (called "wear bar" by dev wiki) color cannot be customized by modders, and is listed under hardcoded features. This PR fixes that by adding fields in item def as well as item metadata to change the appearance. (See lua_api.txt for api specifics)
Resolves #13325

Summary

The wear_bar field in item definitions and item meta allows modders to customize the color of wear bars in a variety of ways.

To do

This PR is Ready for Review.

How to test

In the devtest game, grab a few of the pickaxes (5, 10, 50, and 100 use pickaxes are customized) and dig some stone, allowing their wear bars to appear. Note the progression through different colors (as described by the tool description) when the pickaxe takes more damage.
Additionally, there is a pickaxe called "Wear Bar Color Test," that when punched with will use a random color for its wear bar. You can use the /use_tool command to damage the tool a bit in order to allow the wear bar to appear.

Screenshot demonstrating the different color possibilities:
minetest_wear_bar_color_explanation

@wsor4035 wsor4035 added @ Script API Feature ✨ PRs that add or enhance a feature labels Mar 16, 2023
@Wuzzy2
Copy link
Contributor

Wuzzy2 commented Mar 20, 2023

Nice. I like it when things get un-hardcoded in Minetest. Could be useful for technic tools which use the wear bar for a charge level.

I did not test but I looked at the documentation. I strongly suggest to explicitly explain the purpose of wear_color in the item definition in a single sentence here. This cross-refererencing stuff is confusing and inconvenient.

Suggestions for future PRs:

  • Change color of background
  • Allow to hide bar entirely

@FossFanatic
Copy link

FossFanatic commented Mar 20, 2023

I'd add wear bar shadows to the list as well.

Wear Bar Shadow

It's a small detail, but it looks very nice.

@phseiff
Copy link

phseiff commented Apr 8, 2023

I also think that'd be a great feature, since what is critically high wear (which is basically what the red/yellow color of the wear bar is supposed to indicate) can vary greatly based on what gameplay mechanism the item/ item wear bar is used for and what the wear bar is meant to indicate.

For example if the wear bar is used to indicate temperature (e.g. for a thermometer item) then having it get red when it gets low doesn't make a lot of sense, so allowing modders to change wear bar color for an item via metadata would open a lot of possibilities for how mods can use the little bar under an item.

doc/lua_api.txt Outdated Show resolved Hide resolved
games/devtest/mods/basetools/init.lua Outdated Show resolved Hide resolved
@wsor4035 wsor4035 added the Rebase needed The PR needs to be rebased by its author. label May 26, 2023
@rubenwardy rubenwardy added Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. Concept approved Approved by a core dev: PRs welcomed! and removed Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. labels May 27, 2023
@Zughy Zughy removed the Rebase needed The PR needs to be rebased by its author. label May 28, 2023
Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

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

Please rebase this PR (do not merge branch) to current master branch. It should only consists of your commits (around 5 or 7 in this PR). You can read more here.

@srifqi srifqi added the Rebase needed The PR needs to be rebased by its author. label May 29, 2023
@techno-sam techno-sam force-pushed the wear_bar_api branch 2 times, most recently from 1bef033 to 1acd2a3 Compare May 29, 2023 14:18
@techno-sam
Copy link
Contributor Author

Thanks for the link! Being not super experienced with git (I tend to just have 1 or 2 branches on personal projects), I didn't think of the fact that if I didn't force push, the history on github would clash with my rebased history, and so I accidentally did a merge I shouldn't have. All fixed now.

@srifqi srifqi removed the Rebase needed The PR needs to be rebased by its author. label May 29, 2023
@srifqi srifqi self-requested a review May 29, 2023 22:40
Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

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

The concept and implementation look good. I have also tested it, but I have not checked its performance. There are some code styles fix needed, though.

doc/lua_api.md Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
src/itemstackmetadata.cpp Outdated Show resolved Hide resolved
src/tool.cpp Outdated Show resolved Hide resolved
src/tool.cpp Outdated Show resolved Hide resolved
src/script/common/c_content.cpp Outdated Show resolved Hide resolved
src/script/common/c_content.h Outdated Show resolved Hide resolved
@srifqi srifqi added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label May 30, 2023
Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

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

Just a few changes needed. Sorry for my mistakes in the last review.

doc/lua_api.md Outdated Show resolved Hide resolved
games/devtest/mods/basetools/init.lua Outdated Show resolved Hide resolved
@techno-sam
Copy link
Contributor Author

No problem at all, I really appreciate that you're spending time looking at it.

Copy link
Member

@srifqi srifqi left a comment

Choose a reason for hiding this comment

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

This PR looks good to me. Further improvements can be added later.

@srifqi srifqi added One approval ✅ ◻️ and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Jun 3, 2023
@srifqi srifqi added the Rebase needed The PR needs to be rebased by its author. label Jul 5, 2023
@Zughy
Copy link
Member

Zughy commented Jul 24, 2023

@techno-sam rebase needed (we forgot to tell you, sorry!)

@techno-sam
Copy link
Contributor Author

No problem!
I'll rebase tonight when I have time.

@techno-sam
Copy link
Contributor Author

Done @Zughy

techno-sam and others added 7 commits January 24, 2024 08:42
Co-authored-by: Muhammad Rifqi Priyo Susanto <muhammadrifqipriyosusanto@gmail.com>
Co-authored-by: Muhammad Rifqi Priyo Susanto <muhammadrifqipriyosusanto@gmail.com>
Co-authored-by: Muhammad Rifqi Priyo Susanto <muhammadrifqipriyosusanto@gmail.com>
Co-authored-by: Muhammad Rifqi Priyo Susanto <muhammadrifqipriyosusanto@gmail.com>
new, simplified API for wear bar coloring
@Zughy Zughy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jan 24, 2024
Copy link
Contributor

@appgurueu appgurueu left a comment

Choose a reason for hiding this comment

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

Refactored a bit, please review @techno-sam (and @srifqi) to make sure you're still happy with it. Tested, everything works as expected. Thank you very much for the good work!

doc/lua_api.md Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 1, 2024
@grorp
Copy link
Member

grorp commented Feb 2, 2024

Another documentation thing: This PR adds "Wear Bar Color" as a separate top-level section in lua_api.md, akin to "Games" or "Nodes". Please move the newly added documentation into an existing top-level section instead (e.g. into "Definition tables").

@techno-sam
Copy link
Contributor Author

@appgurueu thanks for your refactor. I'm afraid I forgot to clean up after myself when I condensed the API.

src/client/hud.cpp Outdated Show resolved Hide resolved
@grorp
Copy link
Member

grorp commented Feb 2, 2024

Using wear values directly instead of "durability" values would still be more consistent IMO (what do others think?)

However, if we decide to use "durability" values for whatever reason, it would be good to add a definition of "durability" as 1.0 - (wear / 65535) to lua_api.md, since this term isn't used anywhere else.

@grorp grorp removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Feb 2, 2024
@appgurueu appgurueu merged commit 176e674 into minetest:master Feb 2, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature @ Script API >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tool Damage Bar Colors are Hardcoded