Fix tool rules: Ensure get_speed and is_correct_for_drops check for tags #566
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…for tags Both functions were ignoring tags (`#tag`), causing incorrect default values. - Now correctly checks tags before returning speed or drop validity. - Ensures tool rules work as expected for both direct and tagged block matches.
replace manual stripping with strip_prefix
Make clippy happy and use strip_prefix instead of manual stripping. Also removing unnecessary reference
…yn/Pumpkin into block_breaking_calculations
kralverde
requested changes
Feb 21, 2025
| continue; | ||
| // No tool? Use default speed | ||
| let Some(tool) = &self.item.components.tool else { | ||
| return 1.0; |
There was a problem hiding this comment.
You should pull out the default speed to a constant value to avoid magic numbers
Comment on lines
+71
to
+95
| // 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
This can all be de-duplicated by using the above suggestion
Comment on lines
+41
to
+60
| 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('#') { | ||
| // 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
This should be a method associated with ToolComponent
| return speed; | ||
| } | ||
|
|
||
| if entry.starts_with('#') { |
There was a problem hiding this comment.
This can be done better as:
match entry.strip_prefix("#") {
Some(blocks) => ...
None => (check equality of block with entry)
}
|
Can you may upstream master so clippy is happy with the master code |
I'll update everything on Monday / Tuesday as I'm currently abroad. I'll also upstream master code ^^ |
|
I want to have this merged so i can move on with effects, Further optimations can be applied later. Thank you @Jayryn 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes an issue where get_speed and is_correct_for_drops were not checking for tagged blocks (#tag), causing them to return incorrect default values (1.0 for speed and false for drops). This change is necessary to mitigate block breaking miscalculation by returning wrong speed or items not being dropped when mining. I am unaware of any issues or limitations with this as of now. It worked correctly in a testing environment.
Changes
✅ Fixed get_speed: Now correctly retrieves the speed from tagged blocks instead of always defaulting to 1.0.
✅ Fixed is_correct_for_drops: Properly checks if a block belongs to a tag before returning false.
✅ Updated documentation: Improved inline comments for clarity.
How to Review
• Check that the updated logic correctly looks up tagged blocks.
• Ensure function behavior is consistent with the intended tool rules.
I am unaware of any open issues that relate to this PR.
Please follow our Coding Guidelines