Add Cake #81

Merged
TheBrokenRail merged 22 commits from bigjango13/minecraft-pi-reborn:master into master 2024-02-07 06:47:47 +00:00
Contributor

Adds cake, crafting remainders, milk buckets, death messages, misc_run_on_language_setup, and a lot more symbols.

Adds cake, crafting remainders, milk buckets, death messages, `misc_run_on_language_setup`, and a lot more symbols.
bigjango13 added 7 commits 2024-01-28 19:20:39 +00:00
- Milk Buckets can be obtained by using an empty bucket on a cow
- They can be drunk, but they don't heal you
- Adds the `misc_run_on_language_setup` function
- Changes `misc_run_on_tile_setup` and `misc_run_on_item_setup` to run after the original function is called
- Fix used items transferring durability
- Add more symbols
- Revert `misc_run_on_tile_setup` running after the original function is called, as it broke custom tiles.
- More symbols
bigjango13 added 1 commit 2024-01-28 19:32:31 +00:00
Author
Contributor

I messed up the merge, fixing it now.

I messed up the merge, fixing it now.
bigjango13 added 1 commit 2024-01-28 20:04:48 +00:00
TheBrokenRail reviewed 2024-01-28 22:50:38 +00:00
@ -7,1 +7,4 @@
* Added ``overwrite_calls_within`` Function
* Add ``Add Cake`` Feature Flag (Enabled By Default)
* Add Milk Buckets
* Implement Crafting Remainders

Why did you add this to the v2.5.3 changelog?

Why did you add this to the v2.5.3 changelog?
Author
Contributor

Should I add it to v2.5.4 or v2.6.0 instead?

Should I add it to v2.5.4 or v2.6.0 instead?

Add it to v3.0.0.

Add it to v3.0.0.
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-01-28 22:52:02 +00:00
@ -7,3 +7,3 @@
// Items
static Item *bucket = NULL;
static FoodItem *bucket = NULL;

Do we really care about MCPE compatibility that much? IMO, the code might be nicer if the milk bucket was just a separate item. What are your thoughts?

Do we really care about MCPE compatibility that much? IMO, the code might be nicer if the milk bucket was just a separate item. What are your thoughts?
Author
Contributor

What MCPE compatibility? As far as I know, each type of bucket has it's own id (326 water bucket, 327 lava bucket, and 335 milk bucket). While it might be a little bit nicer without all the aux value checks, it would also be a lot more verbose (even with just splitting off milk buckets), and I think that the current systems is a bit harder to understand the first time, but once it's understood it's a lot easier to read/work with than splitting it off. (It also takes up 1-3 less item ids, but that's a pretty minor bonus.)

What MCPE compatibility? As far as I know, each type of bucket has it's own id (326 water bucket, 327 lava bucket, and 335 milk bucket). While it might be a little bit nicer without all the aux value checks, it would also be a lot more verbose (even with just splitting off milk buckets), and I think that the current systems is a bit harder to understand the first time, but once it's understood it's a lot easier to read/work with than splitting it off. (It also takes up 1-3 less item ids, but that's a pretty minor bonus.)
TheBrokenRail reviewed 2024-01-28 22:53:05 +00:00
@ -119,0 +135,4 @@
static void BucketItem_useTimeDepleted(FoodItem *item, uchar *param_1, ItemInstance *item_instance, Level *level, Player *player) {
if (item_instance->auxiliary == 1) {
(*FoodItem_useTimeDepleted_vtable_addr)(item, param_1, item_instance, level, player);

Use FoodItem_useTimeDepleted_non_virtual rather than de-referencing the address yourself.

Use `FoodItem_useTimeDepleted_non_virtual` rather than de-referencing the address yourself.
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-01-28 22:53:38 +00:00
@ -139,2 +196,3 @@
FoodItem *item = alloc_FoodItem();
ALLOC_CHECK(item);
Item_constructor(item, id);
Item_constructor((Item *) item, id);

Does FoodItem not have its own constructor?

Does `FoodItem` not have its own constructor?
Author
Contributor

I think it was inlined, it must have been pretty minor, as I can't find any trace of it in minecraft-pi or libminecraftpe.so

I think it was inlined, it must have been pretty minor, as I can't find any trace of it in minecraft-pi or libminecraftpe.so
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-01-28 22:54:19 +00:00
@ -0,0 +5,4 @@
#include <mods/init/init.h>
#include <mods/misc/misc.h>
Tile *cake = NULL;

This should probably be static.

This should probably be `static`.
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-01-28 22:55:24 +00:00
@ -0,0 +166,4 @@
cake_instance->count = 255;
cake_instance->auxiliary = 0;
cake_instance->id = 92;
(*FillingContainer_addItem)(filling_container, cake_instance);

Remove the (* and ).

Remove the `(*` and `)`.
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-01-28 22:56:33 +00:00
@ -0,0 +234,4 @@
if (feature_has("Add Cake", server_enabled)) {
misc_run_on_tiles_setup(Tile_initTiles_injection);
misc_run_on_creative_inventory_setup(Inventory_setupDefault_FillingContainer_addItem_call_injection);
if (feature_has("Add Buckets", server_enabled)) {

feature_has should ideally only run once per feature (to avoid redundant log entries). Maybe bucket could set a variable if it's enabled?

`feature_has` should ideally only run once per feature (to avoid redundant log entries). Maybe `bucket` could set a variable if it's enabled?
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-01-28 22:56:54 +00:00
@ -103,1 +101,4 @@
Item_initItems();
// Run Functions
handle_misc_items_setup(NULL);

Why was the order of Item_initItems_injection changed?

Why was the order of `Item_initItems_injection` changed?
Author
Contributor

A few reasons:

  • It allows for mods to modify and overwrite existing items
  • It allows for mods to tell if an item is already assigned to an id
  • It doesn't really change anything beyond that (the same isn't true for tiles)
A few reasons: - It allows for mods to modify and overwrite existing items - It allows for mods to tell if an item is already assigned to an id - It doesn't really change anything beyond that (the same isn't true for tiles)
TheBrokenRail reviewed 2024-01-28 22:58:29 +00:00
@ -548,0 +555,4 @@
if (selected_slot != self->inventory->selectedSlot) {
self->itemBeingUsed.id = 0;
}
Player_stopUsingItem(self);

Rather than all this, why not just make the player stop using the item when the selected slot is changed?

Rather than all this, why not just make the player stop using the item when the selected slot is changed?
Author
Contributor

That already happens, however if itemBeingUsed.id == heldItem.id, heldItem's auxiliary will be set to that of itemBeingUsed. This fixes it by insuring that itemBeingUsed's id cannot be the same, as empty slots are (almost1) always NULL instead of having an id of 0.


  1. There are a few way to obtain an EmptyItemInstance, however it's throught bugs/mods and it almost always crashes within a few seconds of being held. ↩︎

That already happens, however if `itemBeingUsed.id == heldItem.id`, `heldItem`'s `auxiliary` will be set to that of `itemBeingUsed`. This fixes it by insuring that `itemBeingUsed`'s `id` cannot be the same, as empty slots are (almost[^1]) always NULL instead of having an id of 0. [^1]: There are a few way to obtain an EmptyItemInstance, however it's throught bugs/mods and it almost always crashes within a few seconds of being held.
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-01-28 23:00:49 +00:00
@ -61,0 +71,4 @@
LocalPlayer *player = self->minecraft->player;
if (!player->inventory->vtable->add(player->inventory, craftingRemainingItem)) {
// Drop
player->vtable->drop(player, craftingRemainingItem, false);

How does this work if, for instance, the remaining item is 5 sticks and the player's inventory has space for 3 sticks? Do 3 sticks get added to the inventory and 2 dropped?

How does this work if, for instance, the remaining item is 5 sticks and the player's inventory has space for 3 sticks? Do 3 sticks get added to the inventory and 2 dropped?
Author
Contributor

Yep!

Yep!
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-01-28 23:03:47 +00:00
@ -61,0 +83,4 @@
if (self->craftingRemainingItem != NULL) {
ItemInstance *ret = alloc_ItemInstance();
ret->id = self->craftingRemainingItem->id;
ret->count = item_instance->count;

This feels wrong? Was this added for a specific reason?

This feels wrong? Was this added for a specific reason?
Author
Contributor

Yes, in case anyone decides to use Item's craftingRemainingItem instead of overwriting it's Item_getCraftingRemainingItem. I mentioned it here.

Yes, in case anyone decides to use `Item`'s `craftingRemainingItem` instead of overwriting it's `Item_getCraftingRemainingItem`. I mentioned it [here](https://discord.com/channels/740287937727561779/761048906242981948/1200982540501135401).
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-01-28 23:05:15 +00:00
@ -0,0 +1,5 @@
method void craftSelectedItem() = 0x2e0e4;

Shouldn't this extend Screen?

Shouldn't this extend `Screen`?
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-01-28 23:12:31 +00:00
@ -12,0 +12,4 @@
// Normally returns 0
virtual-method int getUseDuration(ItemInstance *item_instance) = 0x24;
// I don't know much about param_1, it might be some partially initialized ItemInstance*
virtual-method void useTimeDepleted(uchar *param_1, ItemInstance *item_instance, Level *level, Player *player) = 0x28;

This seems wrong. According to Ghidra, this function's signature is Item::useTimeDepleted(ItemInstance*, Level*, Player*) (and of course an extra Item *self).

It could be returning a structure, IIRC if a function returns a structure, the caller allocates the structure and passes a pointer to it as the first argument top the function (like how this is passed) and then the function writes to that pointer. Check out CommandServer::parse in Ghidra.

This seems wrong. According to Ghidra, this function's signature is `Item::useTimeDepleted(ItemInstance*, Level*, Player*)` (and of course an extra `Item *self`). It could be returning a structure, IIRC if a function returns a structure, the caller allocates the structure and passes a pointer to it as the first argument top the function (like how `this` is passed) and then the function writes to that pointer. Check out `CommandServer::parse` in Ghidra.
Author
Contributor

Hmm, seems like it. it tripped me up because both Item and ItemInstance have the id at 0x4.

Hmm, seems like it. it tripped me up because both Item and ItemInstance have the id at 0x4.
bigjango13 marked this conversation as resolved
bigjango13 added 1 commit 2024-01-29 00:31:28 +00:00
bigjango13 force-pushed master from a5e223859c to c93350a44c 2024-01-30 03:18:10 +00:00 Compare
bigjango13 added 1 commit 2024-01-30 03:44:21 +00:00
TheBrokenRail reviewed 2024-01-30 03:58:25 +00:00
@ -1 +1 @@
Subproject commit 8e6c8d7effc54f8aecd30eda17069588298f4ada
Subproject commit b4c3ef9d0fdf46845f3e81e5d989dab06e71e6c1

Why did you downgrade GLFW?

Why did you downgrade GLFW?
Author
Contributor

I have no idea, I just git pulled, and when I git added it was there. How would I undo it?

I have no idea, I just git pulled, and when I git added it was there. How would I undo it?
Author
Contributor

Figured it out

Figured it out
bigjango13 marked this conversation as resolved
@ -18,0 +40,4 @@
}
} else if (aux) {
// Throwable with owner
Level *level = player->level;

Shouldn't this check if this was an arrow? What if another entity has aux data?

Shouldn't this check if this was an arrow? What if another entity has aux data?
Author
Contributor

That's intended, only throwables have aux values:

// The owner entity id for arrows/throwables, else 0
virtual-method int getAuxData() = 0xf4;

So if I egg you to death, it still says that I'm the one who did it.

That's intended, only throwables have aux values: ``` // The owner entity id for arrows/throwables, else 0 virtual-method int getAuxData() = 0xf4; ``` So if I egg you to death, it still says that I'm the one who did it.
bigjango13 marked this conversation as resolved
@ -18,0 +45,4 @@
return get_death_message(player, shooter, true);
} else if (80 <= type_id || type_id <= 82) {
// Throwable without owner
message += " was shot under mysterious circumstances";

A lot of these death messages are duplicated. Is there any chance these could be consolidated into fewer if statements?

A lot of these death messages are duplicated. Is there any chance these could be consolidated into fewer if statements?
Author
Contributor

Done 👍

Done 👍
bigjango13 marked this conversation as resolved
@ -18,0 +51,4 @@
message += " was blown apart";
} else if (cause->vtable->isHangingEntity(cause)) {
// Painting?
message += " admired too much art";

When would this ever happen?

When would this ever happen?
Author
Contributor

No idea, I thought it might be useful in case the paintings ever try to take over.

No idea, I thought it might be useful in case the paintings ever try to take over.
@ -23,2 +86,3 @@
// Death Message Logic
#define Player_actuallyHurt_injection(type) \
#define Player_death_injections(type) \
static void type##Player_die_injection(type##Player *player, Entity *cause) { \

IIRC when originally implementing death message, that hooking Player::die caused some issues on servers.

Did you test:

  • Host player dying (in LAN)
  • Guest player dying (in LAN)
  • Server killing player (kill console command)
IIRC when originally implementing death message, that hooking `Player::die` caused some issues on servers. Did you test: - Host player dying (in LAN) - Guest player dying (in LAN) - Server killing player (`kill` console command)
Author
Contributor

That's why I also hooked actuallyHurt as it did before. There are quite a few ways to die without calling Player::die, falling into the void is a notable one. If the player dies without calling Player::die, it will just post the usual " has died". As for your question, yes, all of those have been tested.

That's why I also hooked `actuallyHurt` as it did before. There are quite a few ways to die without calling `Player::die`, falling into the void is a notable one. If the player dies without calling `Player::die`, it will just post the usual "<username> has died". As for your question, yes, all of those have been tested.
bigjango13 marked this conversation as resolved
bigjango13 added 1 commit 2024-01-30 04:16:05 +00:00
Author
Contributor

Seems like TNT shirks responsibility for blowing you up, should be an easy patch. Should it be enabled even when death messages aren't? If so, should it be in misc.h?

Seems like TNT shirks responsibility for blowing you up, should be an easy patch. Should it be enabled even when death messages aren't? If so, should it be in misc.h?

If it's only used for death messages, I would put the patch with the death message code. Also do Creepers have the correct death message when blowing players up?

If it's only used for death messages, I would put the patch with the death message code. Also do Creepers have the correct death message when blowing players up?
bigjango13 added 1 commit 2024-01-30 06:00:01 +00:00
Author
Contributor

Sounds good 👍

Also do Creepers have the correct death message when blowing players up?

Yes "FG6 was killed by a Creeper", the only explosion source other than TNT are beds, but:

  • It can only be triggered with mods
  • Beds aren't an entity

Other than that, all death messages work.

Sounds good 👍 > Also do Creepers have the correct death message when blowing players up? Yes "FG6 was killed by a Creeper", the only explosion source other than TNT are beds, but: - It can only be triggered with mods - Beds aren't an entity Other than that, all death messages work.
bigjango13 added 1 commit 2024-01-30 16:10:11 +00:00
bigjango13 added 3 commits 2024-02-02 00:21:23 +00:00
bigjango13 added 1 commit 2024-02-02 00:23:02 +00:00
bigjango13 added 1 commit 2024-02-02 00:26:15 +00:00
TheBrokenRail reviewed 2024-02-06 23:22:19 +00:00
@ -0,0 +1 @@
bool buckets_enabled();

Please add a #pragma once.

Also, instead of a fancy method containing a static bool, why not just use an extern bool buckets_enabled, or just have buckets_enabled() check a variable.

bool buckets_enabled() {
    static bool ret = feature_has("Add Buckets", server_enabled);
    return ret;
}

just seems a bit over-complicated.

Please add a `#pragma once`. Also, instead of a fancy method containing a `static bool`, why not just use an `extern bool buckets_enabled`, or just have `buckets_enabled()` check a variable. ```c++ bool buckets_enabled() { static bool ret = feature_has("Add Buckets", server_enabled); return ret; } ``` just seems a bit over-complicated.
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-02-06 23:25:04 +00:00
@ -264,14 +336,25 @@ static void FurnaceTileEntity_tick_ItemInstance_setNull_injection(ItemInstance *
}
}
static void Language_injection(__attribute__((unused)) void *null) {

This should probably have a comment before it.

This should probably have a comment before it.
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-02-06 23:31:53 +00:00
@ -60,1 +134,4 @@
overwrite_calls((void *) Mob_hurt_non_virtual, (void *) Mob_hurt_injection);
}
// Fix TNT
unsigned char cpy_r1_r0_patch[4] = {0x00, 0x10, 0xa0, 0xe1}; // "cpy r1,r0"

So, uh, what does these patches actually do? Does it set an entity ID somewhere? Because that should probably be in the comment.

So, uh, what does these patches actually do? Does it set an entity ID somewhere? Because that should probably be in the comment.
bigjango13 marked this conversation as resolved
TheBrokenRail reviewed 2024-02-06 23:37:20 +00:00
@ -0,0 +1,7 @@
extends Item;
size 0x30;

FoodItem is 3 32-bit ints bigger than Item, but only one is accounted for nutrition. What are the others? This is important because FoodItem is constructed manually and therefore anything not set manually will be left uninitialized.

`FoodItem` is 3 32-bit ints bigger than `Item`, but only one is accounted for `nutrition`. What are the others? This is important because `FoodItem` is constructed manually and therefore anything not set manually will be left uninitialized.
bigjango13 marked this conversation as resolved
bigjango13 added 1 commit 2024-02-07 01:04:13 +00:00
bigjango13 added 1 commit 2024-02-07 01:17:25 +00:00

Time for the big green button!

Time for the big green button!
bigjango13 added 1 commit 2024-02-07 06:46:35 +00:00
TheBrokenRail merged commit f5a680af7b into master 2024-02-07 06:47:47 +00:00
TheBrokenRail referenced this issue from a commit 2024-02-07 06:47:48 +00:00
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: minecraft-pi-reborn/minecraft-pi-reborn#81
No description provided.