Fix "proper" expressions not being parsed in default target priorities#7138
Fix "proper" expressions not being parsed in default target priorities#7138lL1l1 wants to merge 7 commits into
Conversation
You can reload the weapon.lua file to reload the initial prio parsing
The bug is that the third argument `plain` being `true` disables the pattern matching facilities so the escape character `%` becomes an actual matched character, which causes the string from target priorities to not match. I also regex searched all files with `find\(.*, true\)` and didn't find any other instances of this mistake.
lL1l1
added
type: bug
area: balance
lL1l1
added
area: sim
This reverts commit 0dc3e47.
📝 WalkthroughWalkthroughThis pull request fixes target priority parsing by normalizing syntax from parenthesized wildcard expressions to space-separated category strings. The parser change enables richer category matching, and unit blueprints are updated to use the new format. ChangesTarget Priority Parsing Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Normal parsing will still take in the words that it detects as categories and apply unions to them. So some priorities maybe worked in an unexpected way. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lua/sim/weapon.lua (1)
629-636: 📐 Maintainability & Code Quality | 💤 Low valueConsider updating for consistency (optional).
Line 629 uses pattern-matching search
string.find(v, '%(')while line 58 now uses plain searchStringFind(priority, '(', 1, true). Both correctly detect parentheses, but for consistency and clarity, line 629 could adopt the same plain-search style:if string.find(v, '(', 1, true) then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lua/sim/weapon.lua` around lines 629 - 636, Replace the pattern-based string.find call with a plain search for consistency: in the block that sets cachedPriorities and priorityTable (references: cachedPriorities, priorityTable, ParseEntityCategoryProperly, ParseEntityCategory) change the condition that currently uses string.find(v, '%(') to use a plain search string.find(v, '(', 1, true) so it behaves like the earlier usage and avoids pattern interpretation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lua/sim/weapon.lua`:
- Around line 629-636: Replace the pattern-based string.find call with a plain
search for consistency: in the block that sets cachedPriorities and
priorityTable (references: cachedPriorities, priorityTable,
ParseEntityCategoryProperly, ParseEntityCategory) change the condition that
currently uses string.find(v, '%(') to use a plain search string.find(v, '(', 1,
true) so it behaves like the earlier usage and avoids pattern interpretation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8229cfa-25a2-4c69-8354-e2284e69f446
📒 Files selected for processing (7)
changelog/snippets/fix.7138.mdlua/sim/weapon.luaunits/UAA0204/UAA0204_unit.bpunits/UEA0204/UEA0204_unit.bpunits/URA0204/URA0204_unit.bpunits/XAA0306/XAA0306_unit.bpunits/XSA0204/XSA0204_unit.bp
|
It would be good to have a regression check in blueprints-weapons so that this doesn't happen again, do you think that would be possible? |
|
What do you mean by regression check? I'm not sure how a check in blueprint loading will help with verifying that the code that parses priorities is working (the 'what happened' part of "so that this doesn't happen again"). |
2 participants
Description of the proposed changes
When I was working on #7135, Mercy was not targeting any mobile units on attack move even after I set TargetCheckInterval to a functional value.
The bug was caused by the
string.findthird argument "plain" being set totrue, which disables pattern matching functionality, but the pattern string was written as if pattern matching was enabled. So instead of matching(it was matching%(, since%is a literal character when pattern matching functionality is disabled.By logging the priorities passing through the "proper" parsing path, I identified that the following default priorities have become functional:
Some of these only use unions, so they do not need to use the proper parsing, and I turned them into normal prios. I used
"\(((\w+)\s*\*\s*)+\w+\)""to search files for these prios.Testing done on the proposed changes
The debug logging shows that Mercy's target priority is being "properly" parsed instead of normally.
Mercy auto attacks units when the target check interval is fixed and it is on attack move.
I also regex searched all files with
find\(.*, true\)and didn't find any other instances of this mistake.This is the list of all weapons using proper priorities, excluding the fixed torp bombers:
It is quite a long list so it needs some going over to see if it makes sense. I didn't do so but I trust that it they are reasonable changes.
Additional Context
It is easy to be unaware of this bug because proper prios are used successfully when custom priorities are given, the default prios fix themselves when reset, and the proper prios are generally niche.
Checklist
Summary by CodeRabbit