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

Fix tool rules: Ensure get_speed and is_correct_for_drops check for tags #566

Merged
merged 7 commits into from
Feb 22, 2025
70 changes: 58 additions & 12 deletions pumpkin-world/src/item/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use pumpkin_data::item::Item;
use pumpkin_data::tag::{get_tag_values, RegistryKey};

mod categories;
#[derive(serde::Deserialize, Debug, Clone, PartialEq, Eq)]
Expand Down Expand Up @@ -28,26 +29,71 @@ impl ItemStack {
Self { item_count, item }
}

/// Determines the mining speed for a block based on tool rules.
/// Direct matches return immediately, tagged blocks are checked separately.
/// If no match is found, returns the tool's default mining speed or `1.0`.
pub fn get_speed(&self, block: &str) -> f32 {
if let Some(tool) = self.item.components.tool {
for rule in tool.rules {
if rule.speed.is_none() || !rule.blocks.contains(&block) {
continue;
// No tool? Use default speed
let Some(tool) = &self.item.components.tool else {
return 1.0;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should pull out the default speed to a constant value to avoid magic numbers

};

for rule in tool.rules {
// Skip if speed is not set
let Some(speed) = rule.speed else {
continue;
};

for entry in rule.blocks {
if entry.eq(&block) {
return speed;
}

if entry.starts_with('#') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be done better as:

match entry.strip_prefix("#") {
  Some(blocks) => ...
  None => (check equality of block with entry)
}

// Check if block is in the tag group
if let Some(blocks) =
get_tag_values(RegistryKey::Block, entry.strip_prefix('#').unwrap())
{
if blocks.iter().flatten().any(|s| s == block) {
return speed;
}
}
Comment on lines +41 to +60
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a method associated with ToolComponent

}
return rule.speed.unwrap();
}
return tool.default_mining_speed.unwrap_or(1.0);
}
1.0
// Return default mining speed if no match is found
tool.default_mining_speed.unwrap_or(1.0)
}

/// Determines if a tool is valid for block drops based on tool rules.
/// Direct matches return immediately, while tagged blocks are checked separately.
pub fn is_correct_for_drops(&self, block: &str) -> bool {
if let Some(tool) = self.item.components.tool {
for rule in tool.rules {
if rule.correct_for_drops.is_none() || !rule.blocks.contains(&block) {
continue;
// Return false if no tool component exists
let Some(tool) = &self.item.components.tool else {
return false;
};

for rule in tool.rules {
// Skip rules without a drop condition
let Some(correct_for_drops) = rule.correct_for_drops else {
continue;
};

for entry in rule.blocks {
if entry.eq(&block) {
return correct_for_drops;
}

if entry.starts_with('#') {
// Check if block exists within the tag group
if let Some(blocks) =
get_tag_values(RegistryKey::Block, entry.strip_prefix('#').unwrap())
{
if blocks.iter().flatten().any(|s| s == block) {
return correct_for_drops;
}
}
Comment on lines +71 to +95
Copy link
Contributor

Choose a reason for hiding this comment

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

This can all be de-duplicated by using the above suggestion

}
return rule.correct_for_drops.unwrap();
}
}
false
Expand Down
Loading