Skip to content

Merge with all weapon blueprints that have the given label when using MergeLabel - #7191

Open
lL1l1 wants to merge 5 commits into
developfrom
fix/weapon-merge-by-label
Open

Merge with all weapon blueprints that have the given label when using MergeLabel#7191
lL1l1 wants to merge 5 commits into
developfrom
fix/weapon-merge-by-label

Conversation

@lL1l1

@lL1l1 lL1l1 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Description of the proposed changes

When implementing weapon merging, I assumed weapon labels should be unique, but according to @The-Balthazar they're sometimes used for using the same weapon script class.

Testing done on the proposed changes

none

Checklist

Summary by CodeRabbit

  • Bug Fixes

    • Fixed weapon blueprint merging so MergeLabel now applies to all matching weapon blueprints, rather than only one.
    • Improved consistency when multiple sub-weapons share the same label.
  • Documentation

    • Clarified blueprint weapon-merging behavior in the relevant documentation and changelog.

@lL1l1
lL1l1 requested review from BlackYps and speed2CZ July 24, 2026 08:14
@lL1l1 lL1l1 added area: unit-blueprint related to issues in unit blueprints (*_unit.bp) feature: mod compatibility related to FAF compatibility with mods labels Jul 24, 2026
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Updates weapon blueprint merging to clear processed mod data and documents that MergeLabel applies to matching weapon blueprints.

Changes

Weapon blueprint merging

Layer / File(s) Summary
Merge processing and documented contract
lua/system/Blueprints.lua, engine/Core/Blueprints/WeaponBlueprint.lua, lua/MODS.LUA, changelog/snippets/fix.7191.md
PreModBlueprints() clears bp.ModWeapon after processing, MergeWeaponByLabel() adjusts its merge flow, and documentation states that MergeLabel applies to matching weapon blueprints.
Estimated code review effort: 2 (Simple) ~10 minutes

Suggested labels: area: sim, area: documentation

Suggested reviewers: speed2cz, blackyps

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and accurately summarizes the main change to MergeLabel weapon merging.
Description check ✅ Passed The description covers the change, testing status, and checklist, and is complete enough for the template.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/weapon-merge-by-label

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lL1l1
lL1l1 marked this pull request as ready for review July 24, 2026 08:22

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@lua/system/Blueprints.lua`:
- Around line 1006-1013: Update the weapon-merging flow around BlueprintMerged
so it continues scanning all weapons after a label match instead of returning
immediately. Perform fallback insertion only after the full collection has been
checked and no match was found, and add a regression test covering multiple
weapons sharing the same label.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7665dc0d-9635-4136-8170-b80f9d115521

📥 Commits

Reviewing files that changed from the base of the PR and between 1488b65 and 6494b5c.

📒 Files selected for processing (4)
  • changelog/snippets/fix.7191.md
  • engine/Core/Blueprints/WeaponBlueprint.lua
  • lua/MODS.LUA
  • lua/system/Blueprints.lua

Comment thread lua/system/Blueprints.lua
Comment on lines +1006 to 1013
local merged = false
if w.Label == label then
weaponTable[i] = BlueprintMerged(w, newBp)
merged = true
end
if merged then
return
end

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Continue merging after the first matching weapon.

The immediate return still limits merging to one weapon with the label, so duplicate-label blueprints remain only partially merged. Remove this early return and perform the fallback insertion only when no match was found; add a regression test covering multiple weapons with the same label.

Proposed fix
     for i, w in weaponTable do
-        local merged = false
         if w.Label == label then
             weaponTable[i] = BlueprintMerged(w, newBp)
-            merged = true
-        end
-        if merged then
-            return
+            merged = true
         end
     end
🤖 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/system/Blueprints.lua` around lines 1006 - 1013, Update the
weapon-merging flow around BlueprintMerged so it continues scanning all weapons
after a label match instead of returning immediately. Perform fallback insertion
only after the full collection has been checked and no match was found, and add
a regression test covering multiple weapons sharing the same label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: unit-blueprint related to issues in unit blueprints (*_unit.bp) feature: mod compatibility related to FAF compatibility with mods

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants