Conversation
|
@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. |
| damage: f32, | ||
| properties: [@typeInfo(ProceduralItemProperty).@"enum".fields.len]f32 = @splat(0), | ||
|
|
||
| durability: u32, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We break items only when they are used.
But yes, general intention is to make the item asap.
| }; | ||
| } | ||
|
|
||
| pub fn onLeftClick(self: Item) ItemUsedCallback { |
There was a problem hiding this comment.
I think I would prefer if we followed the pattern we used before where Item functions only call into identical functions of ProceduralItem and BaseItem. This guarantees that when implementations diverge, changes will be limited to those structs.
Its a bit more explicit, but I think its for a good reason.
There was a problem hiding this comment.
is that not what I am already doing? The only wierd thing is the null but for that I have to fallback to the default behavior
There was a problem hiding this comment.
I would expect
.baseItem => |item| item.onLeftClick(),
And call into callback inside impl of onLeftClick
Not sure tho, I had a look at other implementations after making this comment and they are not that consistent.
There was a problem hiding this comment.
Ah. I understand now. Yeah its not consistent, but I will change it.
| item: main.items.Item, | ||
| target: union(enum) { | ||
| block: struct { block: Block, blockPos: Vec3i }, | ||
| entity: *main.server.Entity, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| 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 { |
There was a problem hiding this comment.
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.
| 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| { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
that means callbacks would need a finsish method right? How would they else know that the they can remove the animation
There was a problem hiding this comment.
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.
| var selectionFace: chunk.Neighbor = undefined; | ||
| var lastPos: Vec3d = undefined; | ||
| var lastDir: Vec3f = undefined; | ||
| pub var lastDir: Vec3f = undefined; |
There was a problem hiding this comment.
I don't think it should be made public; this should be a parameter to callback. Preferably inside a struct alongside Callback should receive current player position and direction and should figure out storing that. Then the callback should figure out storing that, no?lastPos as playerOrientation or alike.
| } | ||
|
|
||
| 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 { |
There was a problem hiding this comment.
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.
| var currentBlockProgress: f32 = 0; | ||
| var currentSwingProgress: f32 = 0; | ||
| var currentSwingTime: f32 = 0; | ||
| pub var lastSelectedBlockPos: Vec3i = undefined; |
There was a problem hiding this comment.
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.
This PR will add ItemCallbacks
For now this will only contain
onLeftClickRequires: #2891
What needs to be done before this PR is ready: