Add tool durability bar

[youke1686]

I added the durability bar to tools to replace the original randomly-broken logic.
Earlier discussion is in issues/172

When I was playing Minecraft, I have a principle that only let one of my pickaxes has it's durability bar. in other words, keep the extra tools completely new. But if the tools even don't have the durability bar and just broken randomly, it really makes me uneasy. Sometimes, I even think I was playing Terraria! That's why I made this pull request.

In this pull request, I used the amount of tool (inventory_count field) to store the durability message. In Minecraft, the tools can only have one in each slot, which means the inventory_count field of a tool is always set to a meaningless number 1. I just made the number 1 means the full durability when the number 255 means the tool should broken. It causes no extra memory usage but preformed well.

Accordingly, I also made the following changes:

• defined a list of tools which should have a durability and how many durabilities they should have.
• updated the "S->C Set Container Slot" packet to make it able to send the durability message.
• updated the "C->S Click Container" packet to prevent durability changes when moving tools in container slots

Since the inventory_count field only ranging from 0 to 255, it is not enough to store the exact durability. Considered that what I have added is just a durability bar rather than the number of the durability, I made the durability decreased randomly. While not pixel-perfect, it's statistically accurate. As long as you isn't boring enough to count how many blocks a pickaxe can break, It is completely same as the vanilla Minecraft.

This is my first github pull request. If my pull request is incorrect or has any issues, I’m happy to make corrections.
Thank you for creating and maintaining such an inspiring project. I’d be grateful if you could take a moment to review.

[uncreativeCultist]

it's probably just my odd use-case of a server, but your fork is failing to build due to "unknown type name 'bool'"
is it possible it's not defined or something?

[youke1686]

oh, I think it is my fault.
let me check it.

[youke1686]

I think it is hard to tell who was wrong.
it's seems that bool isn't supported on all versions of GCC.
for some reasons, it hasn't caluse an error on my environment, but it shouldn't being ignored.
I'll fix it soon by using defines like what you adviced.

[youke1686]

@uncreativeCultist
I think I have already fixed the error.
you can try to bulid it again.
thanks for your feedback. it helps me a lot.

[sky64redstone]

I just tried to compile it and received the following error:

src/packets.c: In function ‘sc_setContainerSlot’:
src/packets.c:480:90: error: implicit declaration of function ‘max’ [-Wimplicit-function-declaration]
  480 |       return sc_setContainerSlotWithComponent(client_fd, window_id, slot, 1, item, 1, 3, max(((uint32_t)(count - 1) * get_tool_durability(item)) / 256, 1));
      |                                                                                          ^~~

That's not too bad. I just declared the function in the C file and continued. However, while playing, I found a bug.

If you Shift-click your tool (I have a wooden shovel) from your inventory to your hotbar, and then quickly exit your inventory, the server gets a seg fault. :)

I hope you can figure out why this happens.

But apart from that, I think both the idea and the execution are well done (though I haven't really looked at the code).

[youke1686]

@sky64redstone
Thanks for your feedback. It helped me to find a hidden mistake and solved it, I think.
I am more familiar with C#, but just know a little about C.
It seems that the environment issues are more serious than I thought. I'll keep learning.

I had noticed that my C environment always contains something extra. Since this project had made a point of avoiding these extra functions, I think it must be my mistake to use a wrong environment.
Could you advise on how to obtain a more generic C environment, or a C environment that is more suitable for this project?

[youke1686]

As for the Shift-click bug, I didn't made progress in how to reproduce it.
Here is my build on Windows.
bareiron.zip
I want to know if the bug is in my build too, or it is another environment issue.
I am always open to further feedback.

[sky64redstone]

@youke1686
I'm just going to assume that you've set up your development environment with MinGW and are also using build.sh to compile.
I don't have a Windows computer myself and don't know enough about Windows to be able to help you properly. However, before you upload your code, I would recommend running your compilers with -Wall.
Because I don't have a Windows PC and also because I feel uncomfortable downloading and executing a random file (no offense), I won't check whether the error also occurs in your build. However, I just recompiled your current code and the error is still there.
With -fsanitize=address (which I find very useful), I get the following:
Also don't forget the -g flag, like I just did, to get more information.

AddressSanitizer:DEADLYSIGNAL
=================================================================
==8968==ERROR: AddressSanitizer: SEGV on unknown address 0x01efe9130638 (pc 0x55654613764c bp 0x7ffc51faa020 sp 0x7ffc51fa9ef0 T0)
==8968==The signal is caused by a READ memory access.
    #0 0x55654613764c in cs_clickContainer src/packets.c:679
    #1 0x5565461306be in handlePacket src/main.c:185
    #2 0x556546134175 in main src/main.c:714
    #3 0x7fda4792b6c0  (/usr/lib/libc.so.6+0x276c0) (BuildId: 7a8d41a2df4fde040b4c6ac2832311ab645a1e41)
    #4 0x7fda4792b7f8 in __libc_start_main (/usr/lib/libc.so.6+0x277f8) (BuildId: 7a8d41a2df4fde040b4c6ac2832311ab645a1e41)
    #5 0x55654612a3b4 in _start (/home/sky/dev/c/bareiron-tool-durability/bareiron+0x53b4) (BuildId: a36ecec93066ff3960213e3459de4aee8b6fcf48)

==8968==Register values:
rax = 0x00000f7b489c31c0  rbx = 0x00007bda44d004c0  rcx = 0x0000000000000000  rdx = 0x000001efe9130638
rdi = 0x000000000000036c  rsi = 0x0000000000000002  rbp = 0x00007ffc51faa020  rsp = 0x00007ffc51fa9ef0
 r8 = 0x0000000000000000   r9 = 0x00000f7bc8970100  r10 = 0x0000000000000000  r11 = 0x0000000000000010
r12 = 0x00007bda44d00400  r13 = 0x00000f7b489a0080  r14 = 0x00007ffc51fa9f40  r15 = 0x000055654615bd68
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV src/packets.c:679 in cs_clickContainer
==8968==ABORTING

This means that memory was read in the cs_clickContainer function (at line 679 if (is_tool(*q_item)) {), which the program is not authorized to do.
The error itself is probably something like reading after you've freed up memory space, or reading more from memory than you've reserved space for (like in an array you set an index greater than the arrays length).
Welcome to C, where you sometimes get a hint at whats wrong ;)

It may be that Windows is a little more lenient in this regard and that is why it still works for you, but that is only a guess.
You could try compiling your code with -fsanitize=address and maybe you'll be able to reproduce the SegFault.
Unfortunately, I don't have much time to code anything at the moment :( and therefore won't be very active over the next two or three weeks.

More Information about Address Sanitizer and other usefull build-in debugging features:
GCC Program Instrumentation Options

[youke1686]

oh my goodness!
the cs_clickContainer function is the worst function I've programmed.
it is really a mess. what's worse, it has been a long time sence I programmed it.

I've already spend several hours on it and I'm totally clueless about the crash, espetially at a loss as to how to reproduce this bug.

with chinese winter break ending soon and a mountain of homework to do, I've decided to shelve this bug fix.
I hope I will solve it but surely not now.
I think I've made my best.

anyway, thanks for your feedback. it helps a lot in pinpointing the issue.

[sky64redstone]

Okay, it's basically an easy fix. It just took me some time to understand the code, but it should be as simple as initialising q_item to NULL and checking on line 679 whether q_item is a valid pointer (i.e. not NULL).

[youke1686]

A genious fix!

I confused the packet flow logic last time. when I first saw your fix, I even think it was wrong and things shouldn't act so simple like this.
but after some further research, I find you're right, and it just a dumb mistake on my part.

thanks for this crucial bug fix. this patch was the difference between success and failure for this PR.

[sky64redstone]

Looks good to me 👍🏻