Skip to content

[AAA] Part 3 Item callbacks#2898

Open
Wunka wants to merge 19 commits intoPixelGuys:masterfrom
Wunka:AAA-callbacks
Open

[AAA] Part 3 Item callbacks#2898
Wunka wants to merge 19 commits intoPixelGuys:masterfrom
Wunka:AAA-callbacks

Conversation

@Wunka
Copy link
Copy Markdown
Contributor

@Wunka Wunka commented Apr 12, 2026

This PR will add ItemCallbacks
For now this will only contain onLeftClick

Requires: #2891

What needs to be done before this PR is ready:

  • add the execution of the callback at all relevant places
  • move breaking of blocks in a callback
  • move special wand mechanic into a callback
  • remove testing callback

@Wunka Wunka marked this pull request as ready for review April 13, 2026 19:33
@Wunka Wunka changed the title [AAA] Item callbacks [AAA] Part 3 Item callbacks Apr 13, 2026
@Wunka
Copy link
Copy Markdown
Contributor Author

Wunka commented Apr 14, 2026

@Argmaster I will have to look over the code again to remove some things like the temporary zon creation. But I would like your input for the general structure atleast.

@Argmaster Argmaster self-requested a review April 14, 2026 08:09
Comment thread src/items.zig
damage: f32,
properties: [@typeInfo(ProceduralItemProperty).@"enum".fields.len]f32 = @splat(0),

durability: u32,
Copy link
Copy Markdown
Collaborator

@Argmaster Argmaster Apr 14, 2026

Choose a reason for hiding this comment

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

I think we should consider replacing durability field with timesUsed or something alike, basically reversed durability, so when a tool gets balance changes it doesn't show up funny. Would have to be clamped at zero and tool with zero durability would have to break instantly.

ofc out of scope

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't that mean that for example if a player has an item wich has like 300 timesUsed and we reduce through an update the maxUsage to something below 300, then the item would just break instantly on joining the world.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We break items only when they are used.
But yes, general intention is to make the item asap.

Comment thread src/items.zig
Comment thread src/callbacks/callbacks.zig Outdated
item: main.items.Item,
target: union(enum) {
block: struct { block: Block, blockPos: Vec3i },
entity: *main.server.Entity,
Copy link
Copy Markdown
Collaborator

@Argmaster Argmaster Apr 14, 2026

Choose a reason for hiding this comment

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

I think these may not be mutually exclusive.
A scythe, or a large axe may do AOE damage, what then? (Hitting multiple blocks or entities and blocks)
Should we even do that inside the callback or outside of it?
Additionally it may be very useful to get the source player as a parameter and source inventory too.
Although that last thing has no use for now, so that's something to ignore for now.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Honestly I would remove the target completely and instead provide tools to query the possible target from MeshSelection if the callback cares to do so. This means the params need to be extended with everything required for such query.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed it. callbacks now get the selectedBlockPos which they (if it is not null) can be used to query the block. This also already helped calls because the selection wand doesn't care about the block only its position

Comment thread src/renderer.zig
}

pub fn breakBlock(inventory: main.items.Inventory.ClientInventory, slot: u32, deltaTime: f64) void {
pub fn breakBlock(inventory: main.items.Inventory.ClientInventory, slot: u32, _deltaTime: f64) void {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think you should eliminate the chain of jumps before this call.
Currently you are jumping game.breakBlock -> Inventory.breakBlock -> renderer.MeshSelection.breakBlock -> <onLeftClick callback> for no reason IMO I think it should just call left click callback directly from game.breakBlock.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

but the game.breakBlock has no access to selectedPos I think the call order is ok.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Eh alright, then I guess there is unfortunately a reason. I will have a look at this when I have time to maybe find a better way around this.

Comment thread src/renderer.zig
pub fn breakBlock(inventory: main.items.Inventory.ClientInventory, slot: u32, deltaTime: f64) void {
pub fn breakBlock(inventory: main.items.Inventory.ClientInventory, slot: u32, _deltaTime: f64) void {
var deltaTime = _deltaTime;
if (selectedBlockPos) |selectedPos| {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think breaking animation should be handled inside the callback. You may want to extract it as helper function, but it should be called from inside the callback. There might be tools that don't remove breaking animation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

that means callbacks would need a finsish method right? How would they else know that the they can remove the animation

Copy link
Copy Markdown
Collaborator

@Argmaster Argmaster Apr 15, 2026

Choose a reason for hiding this comment

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

No? I think no.
This is clearing braking animation when you start braking different block.
Since this callback calls into updateBlock sync call, you can do stuff before and after it too, so you can also clear it after brekitn a block.

Comment thread src/renderer.zig Outdated
var selectionFace: chunk.Neighbor = undefined;
var lastPos: Vec3d = undefined;
var lastDir: Vec3f = undefined;
pub var lastDir: Vec3f = undefined;
Copy link
Copy Markdown
Collaborator

@Argmaster Argmaster Apr 14, 2026

Choose a reason for hiding this comment

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

I don't think it should be made public; this should be a parameter to callback. Preferably inside a struct alongside lastPos as playerOrientation or alike. Callback should receive current player position and direction and should figure out storing that. Then the callback should figure out storing that, no?

Copy link
Copy Markdown
Contributor Author

@Wunka Wunka Apr 24, 2026

Choose a reason for hiding this comment

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

is now given over callback

Comment thread src/renderer.zig
}

fn updateBlockAndSendUpdate(source: main.items.Inventory.ClientInventory, slot: u32, pos: Vec3i, oldBlock: blocks.Block, newBlock: blocks.Block) void {
pub fn updateBlockAndSendUpdate(source: main.items.Inventory.ClientInventory, slot: u32, pos: Vec3i, oldBlock: blocks.Block, newBlock: blocks.Block) void {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No you don't. This function makes no sense as part of public API of renderer. Renderer does rendering, it doesn't send updates. Move it or call executeCommand.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have to look how I will exactly handle this.

Comment thread src/renderer.zig Outdated
var currentBlockProgress: f32 = 0;
var currentSwingProgress: f32 = 0;
var currentSwingTime: f32 = 0;
pub var lastSelectedBlockPos: Vec3i = undefined;
Copy link
Copy Markdown
Collaborator

@Argmaster Argmaster Apr 14, 2026

Choose a reason for hiding this comment

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

This should be managed by callback implementation IMO. MeshSelection may have its own use cases for it (does it?) but even then it shouldn't share it with everyone.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

breakBlock seems to use it without reason, in the cases it uses it, it should be the same as selectedPos. I atleast didn't notice any difference in breaking animation.

@Wunka Wunka moved this to WIP/not ready for review in PRs to review Apr 19, 2026
@Wunka Wunka moved this from WIP/not ready for review to High Priority in PRs to review Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: High Priority

Development

Successfully merging this pull request may close these issues.

2 participants