Add nonstacking to all relevant buffs #2623
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
WalkthroughThis pull request introduces a comprehensive refactoring of how non-stacking buffs are handled across multiple artifacts, weapons, and utility modules. The changes primarily focus on implementing a more consistent and flexible approach to managing buffs, particularly emphasizing the implementation of non-stacking mechanics. Key modifications include introducing Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[frontend] [Sun Jan 26 01:14:23 UTC 2025] - Deployed 19a7e59 to https://genshin-optimizer-prs.github.io/pr/2623/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Mon Jan 27 18:35:10 UTC 2025] - Deployed cc83fdb to https://genshin-optimizer-prs.github.io/pr/2623/frontend (Takes 3-5 minutes after this completes to be available) [Mon Jan 27 23:49:37 UTC 2025] - Deleted deployment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (16)
libs/gi/sheets/src/Artifacts/ViridescentVenerer/index.tsx (1)
Line range hint
31-44: Good implementation of team buff non-stacking.The integration of
nonStackBufffor resistance reduction and the team buff structure looks correct. The comment about using the first node as active node is helpful.Consider adding a brief JSDoc comment above the
teamBuffobject to document the non-stacking behavior and its implications:teamBuff: { + /** + * Resistance reduction effects from VV don't stack with other sources. + * Only the first node is used as the active node for each element. + */ premod: objMap(condSwirlNodes, (nodes) => nodes[0]), nonStacking: set4TallyWrites, },libs/gi/sheets/src/Artifacts/TenacityOfTheMillelith/index.tsx (2)
25-29: Check for correct buff scope and naming.Using
nonStackBuff('totm4', 'atk_', percent(0.2))and similarly forshield_is clear. However, confirm that'totm4'is consistently used elsewhere for referencing these non-stacking mechanics, preventing collisions with other sets.
66-71: Confirm user interface implications of inactive states.
set4AtkInactiveandset4ShieldInactiveare exposed in the state fields. Verify that these inactive nodes are displayed correctly in the UI and are only used for debugging or advanced detail. If they’re not intended for normal user display, consider hiding them for clarity.libs/gi/sheets/src/Weapons/Catalyst/HakushinRing/index.tsx (3)
21-22: Refactor naming for clearer intention.
nonElectroEleis concise, but consider a more descriptive name likenonElectroElementsto aid readability, especially for newcomers to the code.
41-45: Avoid potential naming collisions.
'hakushinelectro'and'electro_dmg_'are repeated in the code. Confirm no overlap with other weapon sets or artifact sets using the same ID string.
58-71: Check color-coded rendering for multi-language or theming compatibility.Using
<ColorText>with reaction references is a great UI touch. Verify that it gracefully handles theming, i18n, and any color conflicts for color-blind accessibility.libs/gi/sheets/src/Artifacts/ScrollOfTheHeroOfCinderCity/index.tsx (5)
35-36: Examine combined condition logic.The
isReactEleBuffedapproach sums conditions for both user’s element and direct react condition. Double-check for edge cases where partial conditions might incorrectly award buffs (e.g., multi-element team comps).Also applies to: 38-46
49-54: Assess naming ofset4BaseTallyWrites.
scroll4base${ele}indicates a new property. Ensure the naming is consistent with the rest of the artifact sets to reduce confusion, especially for debugging or logs.
58-60: Consider grouping “condReactBuffs” differently.
nonStackBuffusage is repeated for each element. If the logic grows, grouping or abstracting repeated calls might reduce duplication and improve maintainability.
89-94: Make sure naming conveys meaning.
set4NsTallyWrites,condNightsoulBuffs: Though descriptive, confirm that “Ns” stands for “NightSoul” in all relevant references and that new developers can easily find/understand them.Also applies to: 98-100
104-104: Check key collisions with other artifact data.
totalBuffsmerges multiple damaging buffs by element. Confirm no collisions with other sets or ephemeral states, especially those using similar property names for multi-element coverage.libs/gi/sheets/src/Weapons/Catalyst/StarcallersWatch/index.tsx (2)
18-21: Consider using a more specific buff identifier.While the implementation is correct, consider using a more descriptive identifier than 'starcaller' to better indicate the buff type, such as 'starcaller_shield_dmg' for better clarity and maintainability.
- 'starcaller', + 'starcaller_shield_dmg',
33-35: Update nonStacking property to match the suggested identifier.If you update the buff identifier, ensure consistency here as well.
- starcaller: nonstackWrite, + starcaller_shield_dmg: nonstackWrite,libs/gi/sheets/src/Weapons/Claymore/WolfsGravestone/index.tsx (2)
14-18: Consider using a more specific buff identifier.While the implementation is correct, consider using a more descriptive identifier than 'wolf' to better indicate the buff type, such as 'wolf_team_atk' for better clarity and maintainability.
- 'wolf', + 'wolf_team_atk',
29-31: Update nonStacking property to match the suggested identifier.If you update the buff identifier, ensure consistency here as well.
- wolf: nonstackWrite, + wolf_team_atk: nonstackWrite,libs/gi/sheets/src/Artifacts/Instructor/index.tsx (1)
20-24: Consider adding a comment explaining the tally write logic.While the implementation is correct, adding a comment explaining how
set4TallyWritecombines the artifact set count check and condition check would improve code maintainability.+// Tracks if 4-piece Instructor set is active and the condition is met for the current character const set4TallyWrite = greaterEqStr( input.artSet[key], 4, equalStr(condState, 'on', input.charKey) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
libs/gi/sheets/src/Artifacts/ArchaicPetra/index.tsx(4 hunks)libs/gi/sheets/src/Artifacts/DeepwoodMemories/index.tsx(4 hunks)libs/gi/sheets/src/Artifacts/Instructor/index.tsx(4 hunks)libs/gi/sheets/src/Artifacts/ScrollOfTheHeroOfCinderCity/index.tsx(4 hunks)libs/gi/sheets/src/Artifacts/TenacityOfTheMillelith/index.tsx(4 hunks)libs/gi/sheets/src/Artifacts/ViridescentVenerer/index.tsx(4 hunks)libs/gi/sheets/src/SheetUtil.tsx(1 hunks)libs/gi/sheets/src/Weapons/Bow/ElegyForTheEnd/index.tsx(4 hunks)libs/gi/sheets/src/Weapons/Catalyst/CranesEchoingCall/index.tsx(4 hunks)libs/gi/sheets/src/Weapons/Catalyst/HakushinRing/index.tsx(2 hunks)libs/gi/sheets/src/Weapons/Catalyst/StarcallersWatch/index.tsx(4 hunks)libs/gi/sheets/src/Weapons/Catalyst/ThrillingTalesOfDragonSlayers/index.tsx(4 hunks)libs/gi/sheets/src/Weapons/Claymore/ForestRegalia/index.tsx(4 hunks)libs/gi/sheets/src/Weapons/Claymore/SongOfBrokenPines/index.tsx(4 hunks)libs/gi/sheets/src/Weapons/Claymore/WolfsGravestone/index.tsx(4 hunks)libs/gi/sheets/src/Weapons/Polearm/Moonpiercer/index.tsx(4 hunks)libs/gi/sheets/src/Weapons/Polearm/MountainBracingBolt/index.tsx(1 hunks)libs/gi/sheets/src/Weapons/Sword/FreedomSworn/index.tsx(4 hunks)libs/gi/sheets/src/Weapons/Sword/KeyOfKhajNisut/index.tsx(4 hunks)libs/gi/sheets/src/Weapons/Sword/PeakPatrolSong/index.tsx(4 hunks)libs/gi/sheets/src/Weapons/Sword/SapwoodBlade/index.tsx(4 hunks)libs/gi/wr/src/formula.ts(1 hunks)libs/gi/wr/src/type.d.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: lint
- GitHub Check: call-build / build
🔇 Additional comments (44)
libs/gi/sheets/src/Artifacts/ViridescentVenerer/index.tsx (4)
1-1: LGTM! Import changes align with non-stacking implementation.The new imports support the non-stacking mechanics being implemented.
Also applies to: 5-12
27-30: Well-structured implementation of non-stacking mechanics.The
set4TallyWritesimplementation correctly tracks elemental conditions using the new non-stacking approach. The combination ofgreaterEqStrandequalStrensures proper validation of both artifact set count and elemental swirl conditions.
74-76: Clean implementation of resistance node mapping.The spread operator usage for mapping resistance nodes improves code readability while maintaining functionality.
Line range hint
1-89: Verify consistent implementation across other artifact modules.The non-stacking implementation looks good, but let's verify that similar patterns are used consistently across other artifact modules.
✅ Verification successful
Implementation verified as consistent with other artifact modules
The non-stacking implementation in ViridescentVenerer follows the same patterns found across other artifact modules (ArchaicPetra, DeepwoodMemories, Instructor, etc.), with consistent usage of
nonStackBufffor buff mechanics.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent non-stacking implementation patterns across artifact modules # Look for similar patterns in other artifact modules echo "Checking for nonStackBuff usage patterns:" rg --type typescript "nonStackBuff\(" "libs/gi/sheets/src/Artifacts/" echo -e "\nChecking for teamBuff.nonStacking implementations:" rg --type typescript "teamBuff.*nonStacking" "libs/gi/sheets/src/Artifacts/" echo -e "\nChecking for consistent import patterns:" rg --type typescript "import.*equalStr.*greaterEqStr" "libs/gi/sheets/src/Artifacts/"Length of output: 644
Script:
#!/bin/bash # Description: Check for consistent non-stacking implementation patterns with correct file types echo "Checking for nonStackBuff usage patterns:" rg "nonStackBuff\(" -t ts -t tsx "libs/gi/sheets/src/Artifacts/" -A 2 echo -e "\nChecking for teamBuff.nonStacking implementations:" rg "teamBuff.*nonStacking" -t ts -t tsx "libs/gi/sheets/src/Artifacts/" -A 2 echo -e "\nChecking for consistent import patterns:" rg "import.*\{.*equalStr.*greaterEqStr.*\}" -t ts -t tsx "libs/gi/sheets/src/Artifacts/"Length of output: 626
Script:
#!/bin/bash # Description: Check implementation patterns without file type specifications echo "Checking for nonStackBuff usage patterns:" find libs/gi/sheets/src/Artifacts/ -type f -name "*.tsx" -o -name "*.ts" | xargs grep -l "nonStackBuff" | while read -r file; do echo "=== $file ===" grep -A 2 "nonStackBuff" "$file" done echo -e "\nChecking for teamBuff.nonStacking implementations:" find libs/gi/sheets/src/Artifacts/ -type f -name "*.tsx" -o -name "*.ts" | xargs grep -l "teamBuff.*nonStacking" | while read -r file; do echo "=== $file ===" grep -A 2 "teamBuff.*nonStacking" "$file" done echo -e "\nChecking for relevant imports:" find libs/gi/sheets/src/Artifacts/ -type f -name "*.tsx" -o -name "*.ts" | xargs grep "import.*{.*equalStr.*greaterEqStr.*}"Length of output: 4419
libs/gi/sheets/src/Artifacts/TenacityOfTheMillelith/index.tsx (4)
3-9: Ensure usage consistency of “string-based” comparison utilities.These imports (
equalStr,greaterEqStr) are introduced to handle string-based comparisons. Make sure that their usage is consistent with the rest of the codebase and that no integer-based comparisons remain in logic that expects string-based checks.
10-10: Confirm appropriate usage ofnonStackBuff.
nonStackBuffis imported, presumably to handle artifact buffs that should not stack across multiple sources. Ensure all other calls tononStackBufffollow the same convention for consistency and correctness.
20-21: Validate string comparison approach when gating set bonus.
greaterEqStrcombined withequalStris an interesting approach for verifying both artifact set count and condition node. Verify that it properly evaluates all corner cases (e.g., set level < 4,condNodenot equal to'cast', etc.).Also applies to: 23-23
41-43: Review the nonStacking property for clarity.Adding
totm4: set4TallyWriteis consistent with your approach, but ensure that naming'totm4'is consistent across the entire codebase for referencing Tenacity of the Millelith 4-piece non-stacking logic.libs/gi/sheets/src/Weapons/Catalyst/HakushinRing/index.tsx (6)
2-3: Validate new utility imports.The addition of
objKeyMapandobjKeyValMapcan streamline handling dynamic key-value pairs. Double-check that the rest of the code is updated to use these wherever beneficial, preventing partial adoption and potential confusion.
6-6: Ensure logical correctness in new conditions.
equalStr,unequalStr, andnonStackBuffare introduced. These might change the condition evaluation logic subtly if previously using numeric comparisons. Confirm no unintentional breakage occurred, especially for partial activation or default states.Also applies to: 12-12, 14-14
27-32: Re-examine condition gating logic for correctness.
nonstackWritessets up gating conditions for each element viaobjKeyValMap. Review thatkey === 'electro' ? ... : ...references are correct. A small slip in logic might lead to partial gating or missed buffs for certain elements.
33-39: Confirm synergy between base comparison andnonStackBuff.These lines combine a base
equalcondition withnonStackBuff. Ensure that the function resolves to inactive states appropriately when conditions fail (or are removed). This is crucial for preventing stray buffs from persisting.
52-54: Review extension ofnonStacking: nonstackWrites.This ensures the new approach is recognized as a non-stacking buff source. If multiple weapons or artifacts use similar keys, collisions can occur. Confirm that’s not the case here.
81-101: Validate multi-element reaction states for doping or double-count.These fields generate multi-element damage buffs. Carefully confirm that simultaneous triggers do not incorrectly stack (particularly with
electrovs. non-electro keys).libs/gi/sheets/src/Artifacts/ScrollOfTheHeroOfCinderCity/index.tsx (4)
10-10: Consistency check for new import usage.
equalStr,greaterEqStr,nonStackBuffhave replaced older comparison methods. Confirm they’re used uniformly throughout the file. Intermediate references to old methods could cause inconsistent buff behavior.Also applies to: 12-12, 15-15, 18-18
75-76: Confirm old “reaction on” logic was entirely removed.
isNsEleBuffedchecks for additional conditions. If old logic references remain for reaction states, there could be a mismatch in buff application. Confirm thorough removal or migration of old approach.Also applies to: 78-86
144-154: Cross-check displayed states with newly introduced gating logic.This block enumerates displayed buffs for react and nightsoul conditions. Confirm canShow logic does not cause extra “invisible” buffs in the UI or silently stack.
Also applies to: 182-192
112-115:⚠️ Potential issueValidate correctness of “nonStacking” expansions.
Spreading both
set4BaseTallyWritesandset4NsTallyWritesintononStackingis logical, but ensure that each property remains distinct, or you might inadvertently override a key.- ...set4BaseTallyWrites, - ...set4NsTallyWrites, + ...set4BaseTallyWrites, + ...set4NsTallyWrites,Double-check your final code to confirm no overshadowing occurs.
Likely invalid or redundant comment.
libs/gi/sheets/src/Weapons/Catalyst/CranesEchoingCall/index.tsx (3)
2-3: LGTM! Import changes align with non-stacking implementation.The imports have been correctly updated to include
equalStrandnonStackBuffwhich are essential for the non-stacking mechanics.
13-16: LGTM! Non-stacking buff implementation looks correct.The implementation properly:
- Uses
equalStrto check conditions with character key- Uses
nonStackBuffwith a unique 'crane' identifier- Handles both active and inactive states
25-27: LGTM! Team buff data structure updated correctly.The
nonStackingproperty is properly added toteamBuffwith the 'crane' identifier matching thenonStackBuffimplementation.libs/gi/sheets/src/Weapons/Polearm/MountainBracingBolt/index.tsx (1)
32-33: LGTM! Simplified premod structure.The removal of the unnecessary teamBuff wrapper and direct placement of
skill_dmg_in premod is correct as this buff doesn't require team buff functionality.libs/gi/sheets/src/Weapons/Polearm/Moonpiercer/index.tsx (1)
4-4: LGTM! Well-structured implementation of non-stacking buffs.The changes correctly implement non-stacking mechanics for the Moonpiercer weapon:
- Proper condition checking with
equalStr- Buff calculations using
nonStackBuff- Team buff non-stacking property
- Inactive state tracking
Also applies to: 10-10, 20-24, 32-34, 52-54
libs/gi/sheets/src/Weapons/Sword/SapwoodBlade/index.tsx (1)
4-4: LGTM! Consistent implementation of non-stacking buffs.The changes correctly implement non-stacking mechanics for the Sapwood Blade weapon, following the same pattern as other weapons:
- Proper condition checking with
equalStr- Buff calculations using
nonStackBuff- Team buff non-stacking property
- Inactive state tracking
Also applies to: 10-10, 20-24, 33-35, 52-54
libs/gi/sheets/src/Weapons/Claymore/ForestRegalia/index.tsx (1)
4-4: LGTM! Consistent implementation of non-stacking buffs.The changes correctly implement non-stacking mechanics for the Forest Regalia weapon, following the same pattern as other weapons.
Let's verify this pattern is consistently applied across all weapon files:
Also applies to: 10-10, 20-24, 33-35, 53-55
✅ Verification successful
Verified: Non-stacking buff implementation is consistent across weapon types
The pattern is correctly and uniformly implemented across all weapon files, including:
- Condition checking using
equalStr- Buff calculation using
nonStackBuff- Proper inactive state tracking
- NonStacking property configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent implementation of non-stacking buffs across weapon files # Expected results: All weapon files should show similar patterns of imports and usage # Check for consistent import patterns echo "Checking import patterns..." rg -l "import.*equalStr.*from '@genshin-optimizer/gi/wr'" "libs/gi/sheets/src/Weapons/**/*.tsx" # Check for consistent nonStackBuff usage echo "Checking nonStackBuff usage..." rg "const.*=.*nonStackBuff\(" "libs/gi/sheets/src/Weapons/**/*.tsx" # Check for consistent nonStacking property echo "Checking nonStacking property..." rg "nonStacking:.*{" "libs/gi/sheets/src/Weapons/**/*.tsx"Length of output: 691
Script:
#!/bin/bash # Find all weapon implementation files and verify non-stacking patterns echo "=== Weapon files with equalStr imports ===" fd -e tsx -e ts . "libs/gi/sheets/src/Weapons" --exec rg -l "equalStr.*from.*@genshin-optimizer/gi/wr" echo -e "\n=== NonStackBuff usage with context ===" fd -e tsx -e ts . "libs/gi/sheets/src/Weapons" --exec rg -B 1 -A 2 "const.*=.*nonStackBuff\(" echo -e "\n=== NonStacking property usage with context ===" fd -e tsx -e ts . "libs/gi/sheets/src/Weapons" --exec rg -B 1 -A 2 "nonStacking: \{" || trueLength of output: 3477
libs/gi/sheets/src/Weapons/Catalyst/ThrillingTalesOfDragonSlayers/index.tsx (2)
22-27: LGTM! Non-stacking implementation for TTDS looks good.The implementation correctly handles non-stacking ATK buff using a unique identifier 'ttds' and properly manages both active and inactive states.
39-41: Verify the non-stacking behavior with other ATK buffs.Let's ensure this implementation correctly prevents stacking with other ATK buffs.
✅ Verification successful
Non-stacking behavior is correctly implemented ✓
The implementation uses the standard non-stacking mechanism with the 'atk_' prefix and unique 'ttds' key, ensuring it won't stack with other ATK buffs from sources like Noblesse Oblige, Tenacity of the Millelith, or other weapon effects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other ATK buff implementations that might stack rg -A 5 "nonStackBuff\(['\"].*atk_['\"]" .Length of output: 1114
Script:
#!/bin/bash # Search for nonstackWrite implementation and TTDS buff rg -B 2 -A 5 "nonstackWrite|nonStackBuff.*ttds" . # Also search for the actual ATK buff implementation in TTDS ast-grep --pattern 'const $_ = nonStackBuff($_, $_, $_)'Length of output: 25269
libs/gi/sheets/src/Artifacts/DeepwoodMemories/index.tsx (2)
21-29: LGTM! Non-stacking implementation for Deepwood 4pc effect looks good.The implementation correctly handles non-stacking dendro resistance reduction using a unique identifier 'dm4'.
40-42: Verify the non-stacking behavior with other dendro resistance reduction effects.Let's ensure this implementation correctly prevents stacking with other similar effects.
✅ Verification successful
Verified: Deepwood Memories correctly implements non-stacking dendro resistance reduction.
The implementation properly uses the game's non-stacking buff system to prevent multiple instances of the same effect from stacking. This is consistent with how other resistance reduction effects are handled in the game.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other dendro resistance reduction implementations rg -A 5 "nonStackBuff\(['\"].*dendro_enemyRes_['\"]" .Length of output: 60
Script:
#!/bin/bash # Search for dendro resistance patterns echo "=== Searching for dendro resistance patterns ===" rg -i "dendro.*res" . echo -e "\n=== Searching for resistance reduction implementations ===" rg -i "resistance.*reduction" . echo -e "\n=== Searching for non-stacking patterns ===" rg -i "nonstack" .Length of output: 55936
libs/gi/sheets/src/Weapons/Claymore/SongOfBrokenPines/index.tsx (1)
18-22: LGTM! Non-stacking implementation for Millennial Movement buff looks good.The implementation correctly uses 'millenialatk' identifier to prevent stacking with similar effects from other weapons (e.g., Elegy for the End).
libs/gi/sheets/src/Weapons/Bow/ElegyForTheEnd/index.tsx (2)
23-27: LGTM! Non-stacking implementation for Millennial Movement buff looks good.The implementation correctly uses 'millenialatk' identifier to prevent stacking with similar effects from other weapons (e.g., Song of Broken Pines).
38-40: Verify the non-stacking behavior between Elegy and Song of Broken Pines.Let's ensure the shared 'millenialatk' identifier correctly prevents these weapon buffs from stacking.
✅ Verification successful
Non-stacking behavior between Elegy and Song of Broken Pines is correctly implemented
The shared 'millenialatk' identifier in both weapons' implementations ensures their ATK buffs cannot stack together. The same non-stacking mechanism is also used by Freedom Sworn, making all three "Millennial Movement" series weapons mutually exclusive in terms of their team ATK buffs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all implementations using 'millenialatk' identifier rg "nonStackBuff\(['\"]millenialatk['\"]" . # Search for any other ATK team buff implementations rg -A 5 "teamBuff.*atk_" .Length of output: 21947
Script:
#!/bin/bash # Search for Song of Broken Pines implementation fd "SongOfBrokenPines" . # Search for millenialatk in weapon files rg "millenialatk" . # Search for nonStacking configurations in weapon files rg -A 3 "nonStacking.*:.*{" .Length of output: 3061
Script:
#!/bin/bash # Search for nonstackWrite implementation rg -A 5 "nonstackWrite.*=|const.*nonstackWrite" . # Search for non-stacking related code in formula files rg -A 5 "nonstack.*buff" . --ignore-case # Look for the formula system's buff handling rg -A 5 "registerBuff|addBuff" .Length of output: 82779
libs/gi/sheets/src/SheetUtil.tsx (1)
94-96: LGTM! Type signature enhancement improves function flexibility.The updated parameter type
NumNode | numberallows direct numeric values while maintaining type safety.libs/gi/sheets/src/Artifacts/ArchaicPetra/index.tsx (2)
24-33: LGTM! Robust condition checking for buff application.The implementation correctly ensures that only one element can be buffed at a time using
greaterEqStrwith sum validation.
46-49: Verify the non-stacking implementation across different artifacts.The non-stacking property is correctly implemented, but we should verify consistency with other artifact sets.
✅ Verification successful
Non-stacking implementation is consistent across artifacts ✅
The implementation in ArchaicPetra follows the same pattern used across other artifacts, using
set4TallyWritewith an artifact-specific key. The few variations found (in ViridescentVenerer and ScrollOfTheHeroOfCinderCity) are justified by their specific requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar non-stacking implementations in other artifact files rg -A 3 "nonStacking: {" "libs/gi/sheets/src/Artifacts/"Length of output: 148
Script:
#!/bin/bash # Search for nonStacking property using fixed strings echo "=== Searching for nonStacking property ===" rg -F "nonStacking:" "libs/gi/sheets/src/Artifacts/" -A 3 echo -e "\n=== Searching for possible variations ===" rg -i "non.?stacking" "libs/gi/sheets/src/Artifacts/" -A 3 echo -e "\n=== Searching using AST pattern ===" ast-grep --pattern 'nonStacking: { $$$ }'Length of output: 4511
libs/gi/sheets/src/Weapons/Sword/FreedomSworn/index.tsx (2)
21-25: LGTM! Clear implementation of non-stacking weapon buff.The implementation correctly tracks both the condition and character key for non-stacking behavior.
75-80: Good addition of inactive state visualization.Including both active and inactive states in the UI helps users understand the buff's current status.
libs/gi/sheets/src/Weapons/Sword/KeyOfKhajNisut/index.tsx (2)
52-59: LGTM! Proper handling of HP-scaled non-stacking buff.The implementation correctly combines:
- Non-stacking behavior with
equalStr- HP scaling using
prod- Refinement-based scaling
75-77: Verify the non-stacking implementation across different weapons.The non-stacking property is correctly implemented, but we should verify consistency with other weapons.
✅ Verification successful
The non-stacking implementation is consistent across weapons
The implementation follows the standard pattern used throughout the codebase where each weapon uses
nonStacking: { [key]: nonstackWrite }with a unique identifier key.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar non-stacking implementations in other weapon files rg -A 3 "nonStacking: {" "libs/gi/sheets/src/Weapons/"Length of output: 146
Script:
#!/bin/bash # Search for nonStacking implementations with fixed strings and more context rg -F "nonStacking" "libs/gi/sheets/src/Weapons/" -B 2 -A 4Length of output: 6878
libs/gi/sheets/src/Weapons/Sword/PeakPatrolSong/index.tsx (3)
Line range hint
56-62: LGTM: Non-stacking buff implementation for elemental damage.The implementation correctly uses
nonStackBuffto prevent stacking of the 'patrol' buff with similar buffs from other sources.
72-75: LGTM: Team buff configuration with non-stacking property.The teamBuff configuration correctly includes the non-stacking flag for the 'patrol' buff.
110-113: LGTM: Enhanced node display.The sheet now correctly displays both active and inactive nodes using flatMap, improving visibility of the buff states.
libs/gi/wr/src/type.d.ts (1)
185-185: LGTM: Type definition enhancement for non-stacking buffs.The change to
Partial<Record<NonStackBuff, StrNode>>improves type flexibility by making buff entries optional, which better reflects the actual usage pattern where not all buffs need to be defined.libs/gi/wr/src/formula.ts (1)
49-89: LGTM: Comprehensive expansion of non-stacking buff definitions.The expanded
allNonstackBuffsarray provides a complete list of non-stacking buffs, well-organized by source type:
- Artifact set buffs (totm4, ap4, etc.)
- Element-specific scroll buffs
- Weapon-specific buffs (patrol, key, crane, etc.)
- Element-specific hakushin buffs
The naming convention is consistent and clear.
…nshin-optimizer into van/go/nonStackFull
|
Note that this calculates For example, |
Describe your changes
Add non-stacking mechanic from #2592 to all relevant team buffs
Issue or discord link
Testing/validation
Optimized with various combinations of the non-stacking sets and no crashes were observed.
Optimized for Geo DMG bonus while someone else on the team was providing 40% geo dmg bonus, with scroll artifact set conditional enabled for another 40% geo damage bonus.
Result was a non-4p scroll set, which is expected, as scroll will not provide any additional geo dmg.
Optimized the same, but disabled the teammates GEO dmg bonus.
Result was a 4p scroll set, which is expected, as scroll is now providing the additional geo dmg
Example for weapons of different names, like millenial movement. Here atk% is not stacked
Example for targeted buff/disp node on weapons of different names
Example for targeted buff/disp node on weapons of same name (and also the condition is more complex)
I also tested every artifact/weapon as I implemented it
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-cilocally to validate format and lint.Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
New Features
Improvements
equalStrandnonStackBufffunctions for more precise condition checkingTechnical Updates