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

Improve documentation of tools #11128

Merged
merged 1 commit into from Jul 27, 2021
Merged

Improve documentation of tools #11128

merged 1 commit into from Jul 27, 2021

Conversation

Wuzzy2
Copy link
Contributor

@Wuzzy2 Wuzzy2 commented Mar 27, 2021

This PR changes and adds to the lua_api.txt documentation of tools.

Currently, the documentation of tools is a bit lacking and the actual features are somewhat misrepresented.

It adds a more detailed description of item types to make clear that the term “tool” is really just a technical category and NOT limited to tools in a literal gameplay sense.
The documentation made the false assumption that only tools can do certain things like dig or punch while in reality all items can do that. So, I replaced the word “tool” with “item” where needed.

I also added a description of the toolrepair recipe with the wear formula.

A short explainer of punch attack uses was added.

And a couple of other things (see commit).

To do

This PR is ready to review.

How to test

Read the commit text. ;-)

Copy link
Contributor

@Df458 Df458 left a comment

Choose a reason for hiding this comment

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

Overall this seems like a big improvement to the existing text, I like how it clarifies what actually does/doesn't apply to non-tools. I haven't tested to confirm that this information is accurate, but I assume you've done so on your end.

Besides that, I have a bunch of random nitpicks with the text. Mostly small tweaks, the broad strokes are good imo but a minor editing pass could improve it.

doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Apr 5, 2021

OK, done.

“fulfil” is British English (I don't know which spelling is preferred tho, I don't care).
“identical items” sounds like I'm talking of the same instance of the item, which is not what is meant. Not sure about that.

@Df458
Copy link
Contributor

Df458 commented Jun 29, 2021

Bump

This seems like a pretty trivial PR to review/merge, would be nice if a core dev could give it a quick look.

Copy link
Member

@sfan5 sfan5 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 otherwise.

doc/lua_api.txt Outdated Show resolved Hide resolved
@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jul 1, 2021

I did what sfan5 requested. I rebased the single commit. There was no other change.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Few ideas on how to keep the documentation short but precise.


Why are the toolcaps allowed on items? That was IMO a mistake. Only tools should have tool capabilities.

doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
doc/lua_api.txt Outdated Show resolved Hide resolved
@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jul 20, 2021

Two tools of the same type without the disable_repair=1 group can be combined to a single tool in a shapeless recipe.

This is wrong because it assumes that games always register the toolrepair recipe.

Why are the toolcaps allowed on items? That was IMO a mistake. Only tools should have tool capabilities.

What's wrong with that? I think this is incredibly useful. It makes the engine more flexible. There is only to gain from allowing tool capabilities on all items instead of only tools, and nothing to lose. Also, you will probably break things anyway if you remove this.

The other comments are OK, will work on them soon(TM).

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jul 20, 2021

Oh, I just saw the toolrepair thing referred to the definition of toolrepair ... I thought this was a different section, lol. My bad. I still like to reword that tho.

@Wuzzy2
Copy link
Contributor Author

Wuzzy2 commented Jul 20, 2021

OK, hopefully it's better now.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM

@sfan5 sfan5 merged commit 216728c into minetest:master Jul 27, 2021
Treer added a commit to Treer/minetest that referenced this pull request Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants