ZO add 4p disc effects with simple conditional system #2657
Conversation
WalkthroughThe changes implement a new conditional system for disc 4-piece effects across multiple modules. An overloaded function for object filtering ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User Interface
participant B as BuildsDisplay/Optimize
participant C as Character Context
participant S as Solver
participant W as Worker
participant A as applyCalc Function
U->>B: Initiate optimization action
B->>C: Retrieve character conditionals
C-->>B: Return conditionals data
B->>S: Call onOptimize with conditionals & stats
S->>W: Send optimization message (includes conditionals)
W->>A: Call applyCalc(conditionals, baseStats, discs)
A-->>W: Return calculated stats
W-->>S: Return optimization result
S-->>B: Provide final results
B->>U: Update UI with results
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
|
[zzz-frontend] [Fri Jan 31 03:44:44 UTC 2025] - Deployed e536bfd to https://genshin-optimizer-prs.github.io/pr/2657/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Wed Feb 5 17:50:05 UTC 2025] - Deployed 1c01365 to https://genshin-optimizer-prs.github.io/pr/2657/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Wed Feb 5 19:32:20 UTC 2025] - Deployed 2061e29 to https://genshin-optimizer-prs.github.io/pr/2657/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Thu Feb 6 03:55:52 UTC 2025] - Deployed 9ba94a8 to https://genshin-optimizer-prs.github.io/pr/2657/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Thu Feb 6 03:56:03 UTC 2025] - Deployed 9ba94a8 to https://genshin-optimizer-prs.github.io/pr/2657/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Thu Feb 6 03:56:57 UTC 2025] - Deployed 9ba94a8 to https://genshin-optimizer-prs.github.io/pr/2657/frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Thu Feb 6 04:28:03 UTC 2025] - Deployed 8774f6e to https://genshin-optimizer-prs.github.io/pr/2657/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Thu Feb 6 04:28:16 UTC 2025] - Deployed 8774f6e to https://genshin-optimizer-prs.github.io/pr/2657/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Thu Feb 6 04:29:47 UTC 2025] - Deployed 8774f6e to https://genshin-optimizer-prs.github.io/pr/2657/frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Thu Feb 6 06:51:44 UTC 2025] - Deployed 635ea7f to https://genshin-optimizer-prs.github.io/pr/2657/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Thu Feb 6 06:52:47 UTC 2025] - Deployed 635ea7f to https://genshin-optimizer-prs.github.io/pr/2657/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Thu Feb 6 06:53:50 UTC 2025] - Deployed 635ea7f to https://genshin-optimizer-prs.github.io/pr/2657/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Thu Feb 6 07:04:50 UTC 2025] - Deployed 15a7e8b to https://genshin-optimizer-prs.github.io/pr/2657/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Thu Feb 6 07:05:04 UTC 2025] - Deployed 15a7e8b to https://genshin-optimizer-prs.github.io/pr/2657/frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Thu Feb 6 07:05:19 UTC 2025] - Deployed 15a7e8b to https://genshin-optimizer-prs.github.io/pr/2657/zzz-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Fri Feb 7 03:29:21 UTC 2025] - Deployed de6000b to https://genshin-optimizer-prs.github.io/pr/2657/zzz-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Fri Feb 7 03:29:31 UTC 2025] - Deployed de6000b to https://genshin-optimizer-prs.github.io/pr/2657/frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Fri Feb 7 03:30:02 UTC 2025] - Deployed de6000b to https://genshin-optimizer-prs.github.io/pr/2657/sr-frontend (Takes 3-5 minutes after this completes to be available) [zzz-frontend] [Fri Feb 7 03:56:59 UTC 2025] - Deployed 2d29779 to https://genshin-optimizer-prs.github.io/pr/2657/zzz-frontend (Takes 3-5 minutes after this completes to be available) [sr-frontend] [Fri Feb 7 03:57:16 UTC 2025] - Deployed 2d29779 to https://genshin-optimizer-prs.github.io/pr/2657/sr-frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Fri Feb 7 03:58:22 UTC 2025] - Deployed 2d29779 to https://genshin-optimizer-prs.github.io/pr/2657/frontend (Takes 3-5 minutes after this completes to be available) [Fri Feb 7 05:24:42 UTC 2025] - Deleted deployment |
frzyc
changed the title
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
| baseStats: Stats, | ||
| conditionals: Partial<Record<CondKey, number>>, | ||
| discs: DiscStats[] | ||
| ) { | ||
| const sum = { ...baseStats } | ||
| const s = (key: string) => sum[key] || 0 | ||
|
|
There was a problem hiding this comment.
I think we can remove the objSumInPlace(sum, setCount) now, as the 4p constraint checking is done outside of the calc step
There was a problem hiding this comment.
you are right, but I think i will add some display based on this value for the future, so i'll keep it for now.
| const sum = useMemo( | ||
| () => | ||
| applyCalc( | ||
| baseStats, | ||
| character?.conditionals ?? {}, |
There was a problem hiding this comment.
semi-unrelated to PR:
changing conditionals causes the displayed stat to change on generated builds, but does not change the calculated value.
should we consider just grabbing the final result from the calculation instead of cache?
There was a problem hiding this comment.
the calculated value dictate the order of the build, but you raise a good point. probably something to address for the future for sure.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
libs/zzz/solver/src/solver.ts (1)
78-91: Consider adding type safety for worker message.yawns ... Sorry, been staring at code for too long. The worker message could benefit from a dedicated interface type to ensure type safety.
+interface ParentCommandStartMessage { + baseStats: Stats + conditionals: Partial<Record<CondKey, number>> + command: 'start' + discsBySlot: Record<DiscSlotKey, ICachedDisc[]> + constraints: Constraints + numWorkers: number + formulaKey: FormulaKey + setFilter2: DiscSetKey[] + setFilter4: DiscSetKey[] +} -const message: ParentCommandStart = { +const message: ParentCommandStartMessage = {libs/zzz/solver/src/childWorker.ts (1)
39-39: Fix interface name typo.needs more coffee ... Found a typo in the interface name.
-export interface ChildMessaageResults { +export interface ChildMessageResults {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
libs/common/util/src/lib/object.ts(1 hunks)libs/zzz/consts/src/common.ts(2 hunks)libs/zzz/consts/src/disc.ts(2 hunks)libs/zzz/db/src/Database/DataManagers/CharacterDataManager.ts(6 hunks)libs/zzz/page-optimize/src/BuildsDisplay.tsx(2 hunks)libs/zzz/page-optimize/src/DiscConditionalCard.tsx(1 hunks)libs/zzz/page-optimize/src/Optimize.tsx(1 hunks)libs/zzz/page-optimize/src/StatsDisplay.tsx(1 hunks)libs/zzz/page-optimize/src/index.tsx(2 hunks)libs/zzz/solver/src/calc.ts(3 hunks)libs/zzz/solver/src/childWorker.ts(4 hunks)libs/zzz/solver/src/parentWorker.ts(4 hunks)libs/zzz/solver/src/solver.ts(5 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
libs/zzz/solver/src/calc.ts
[error] 83-83: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
libs/zzz/page-optimize/src/DiscConditionalCard.tsx
[error] 90-90: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
🔇 Additional comments (15)
libs/zzz/consts/src/disc.ts (2)
201-283: New disc condition keys looking solid.
Ugh, haven't slept in 36 hours, but these definitions appear consistent and well-organized. The type-safety and structure look robust from my over-caffeinated vantage point.
358-365: Verify PolarMetal’s stacking logic.
I'm so tired I'm seeing double, but the code literally doubles 28% DMG to 56% when the condition is active. If that’s intended, carry on. If not, tweak the multiplication.libs/zzz/page-optimize/src/BuildsDisplay.tsx (1)
18-19: Importing useCharacterContext looks good.
I can’t rest, but at least this import is neat. Nicely done hooking into the character context.libs/zzz/solver/src/solver.ts (1)
21-22: LGTM! Clean implementation of conditionals in the Solver class.The changes properly integrate the conditional system by adding the necessary property and initializing it in the constructor.
Also applies to: 34-35, 44-45
libs/zzz/consts/src/common.ts (1)
133-134: LGTM! Clean type definition and constant.sips coffee The type alias and constant look good. The use of Object.keys with type assertion is a common pattern.
libs/zzz/page-optimize/src/DiscConditionalCard.tsx (1)
26-52: LGTM! Well-structured parent component.The DiscConditionalsCard component has a clean layout with proper grid system usage.
libs/zzz/solver/src/childWorker.ts (1)
171-173: Consider adding error handling for applyCalc.fighting sleep The applyCalc call could benefit from some error handling in case the conditionals are invalid.
libs/zzz/solver/src/parentWorker.ts (3)
2-6: LGTM! Clean type imports.The imports are well-organized and properly typed.
22-22: LGTM! Type-safe conditionals property.The
conditionalsproperty is properly typed as a partial record, allowing for optional condition values.
190-190: LGTM! Proper propagation of conditionals to child workers.The conditionals are correctly passed to child workers during initialization.
libs/zzz/solver/src/calc.ts (1)
80-87: 🧹 Nitpick (assertive)Consider combining the 4p effect loop with the previous loop.
sips coffee We're iterating over
setCounttwice... yawns We could combine this with the previous loop at lines 73-79 to reduce iterations.Here's how we could combine them:
- // Apply 2p effects - for (const key in setCount) { - const k = key as DiscSetKey - if (setCount[k]! >= 2) { - const p2 = disc2pEffect[k] - if (p2) objSumInPlace(sum, p2) - } - } - // Apply 4p effects - for (const key in setCount) { - const k = key as DiscSetKey - if (setCount[k]! >= 4) { - const p4 = disc4PeffectSheets[k]?.getStats(conditionals, sum) - if (p4) objSumInPlace(sum, p4) - } - } + // Apply 2p and 4p effects + for (const key in setCount) { + const k = key as DiscSetKey + const count = setCount[k]! + if (count >= 2) { + const p2 = disc2pEffect[k] + if (p2) objSumInPlace(sum, p2) + if (count >= 4) { + const p4 = disc4PeffectSheets[k]?.getStats(conditionals, sum) + if (p4) objSumInPlace(sum, p4) + } + } + }Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 83-83: Forbidden non-null assertion.
(lint/style/noNonNullAssertion)
libs/zzz/db/src/Database/DataManagers/CharacterDataManager.ts (2)
46-46: LGTM! Well-typed conditionals property.The
conditionalsproperty is properly typed as a partial record, maintaining type safety.
161-162: 🧹 Nitpick (assertive)Consider adding value range validation for conditionals.
yawns We're validating that values are numbers, but should we also validate their ranges? Some conditions might only make sense within specific bounds.
Consider adding range validation:
- conditionals = objFilter(conditionals, (value) => typeof value === 'number') + conditionals = objFilter(conditionals, (value) => { + if (typeof value !== 'number') return false + return value >= 0 && value <= 1 // or other appropriate range + })✅ Verification successful
🌐 Web query:
What are typical value ranges for game condition toggles?💡 Result:
Game condition toggles typically use **boolean values** (0/1 or true/false) for binary states and **small integer ranges** (e.g., 0-2 or 1-9) for multi-state controls, depending on the complexity of the feature being managed. Here's a breakdown: ### Binary Toggles - **Boolean variables** (true/false) for simple on/off states: - Mute buttons ([3][7][8]) - Kill switches for resource-intensive features ([2][4]) - Basic UI elements like checkboxes ([5][8]) ### Multi-State Toggles - **Number variables** for cycling through multiple options: - Volume levels (e.g., 0 = muted, 1 = half volume, 2 = full volume) [3] - Animation states (e.g., 0–5 for cycling through rune animations) [3][5] - Puzzle mechanics requiring sequential state changes (e.g., block destruction phases) [3] ### Constrained Value Ranges - **Minimum/maximum bounds** for configurable parameters: - Game variables clamped between 1–9 ([1]) - Toggle rates in technical contexts (e.g., signal toggling at 12.5%–25%) [6] ### Implementation Best Practices - Use `clamp()` functions to enforce valid ranges during incremental adjustments (e.g., ensuring `min_range` ≤ `max_range`) [1]. - For dynamic state management, combine **toggle actions** with conditional checks to reset or cycle values (e.g., resetting a counter after reaching its maximum) [3][5]. - Persist toggle states via saved variables (e.g., global booleans stored as numbers) to maintain user preferences across sessions [7]. For example, incrementing a `min_range` variable beyond its paired `max_range` would automatically adjust both values to maintain valid bounds in a single step ([1]). Similarly, a three-state volume toggle might cycle `0 → 1 → 2 → 0` using modular arithmetic ([3][5]). Citations: - 1: https://forum.gamemaker.io/index.php?threads%2Fadjusting-variables-to-control-range-bounds-for-configuration.55186%2F - 2: https://www.abtasty.com/blog/feature-toggle-best-practices/ - 3: https://www.youtube.com/watch?v=vuV_Q_hzACQ - 4: https://martinfowler.com/articles/feature-toggles.html - 5: https://wiki.gdevelop.io/gdevelop5/tutorials/how-to-make-togglable-states-with-variables/ - 6: https://adaptivesupport.amd.com/s/article/36742 - 7: https://forum.gdevelop.io/t/how-to-save-and-load-toggle-button-potation/51236 - 8: https://www.nngroup.com/articles/toggle-switch-guidelines/ - 9: https://www.ultraengine.com/community/topic/4932-toggle-value/ - 10: https://www.intel.com/content/www/us/en/docs/programmable/683174/21-3/toggle-rate.html
Validation Improvement: Consider Explicit Range Checks
The review is spot-on. Right now, we're only filtering by type, but based on typical game toggle conventions, we should also ensure values lie within an expected range—whether that’s strictly binary (0/1) or a small integer range for multi-state controls. Adjusting the filtering to enforce, say, a [0,1] (or other appropriate, context-dependent) range would safeguard against unexpected values.
- Current code only checks for a number.
- Game condition toggles are commonly binary or bound to a small integer range.
- Update the filter (as suggested) to include a range condition—if that aligns with the game logic.
libs/zzz/page-optimize/src/index.tsx (1)
192-192: LGTM! Clean UI integration.sips more coffee The
DiscConditionalsCardis well-placed in the component hierarchy and receives the necessary props.libs/zzz/page-optimize/src/Optimize.tsx (1)
134-147: Looks good, but let me double-check something while I grab another coffee...The addition of
character.conditionalslooks correct, but we should verify how the Solver handles undefined conditionals when character is null.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
libs/common/util/src/lib/object.ts(1 hunks)libs/zzz/consts/src/disc.ts(2 hunks)libs/zzz/solver/src/calc.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (10)
libs/zzz/solver/src/calc.ts (6)
9-9: Ugh, I'm so tired, but this new import looks spot on.
No obvious issues.
17-17: Adding 'disc4PeffectSheets' import.
Looks like it's used properly below—good synergy!
56-60: Fresh conditionals parameter—check callers.
This updated signature is neat, but let's ensure all calls pass in the new parameter correctly.
75-76: Avoiding null!
Finally, no more non-null assertion. The result is safer and won't break in edge cases.
80-80: Comment for 4p effects.
Direct and consistent with the 2p comment. Let’s keep the sense of clarity.
81-84: Applying 4p condition checks.
The code looks stable. Consider adding tests to confirm each disc's synergy.libs/zzz/consts/src/disc.ts (3)
1-1: New import for 'objMultiplication'?
I see that it’s used for scaling effects. All good.
202-283: Defining 'allDiscCondKeys' for 4p logic.
This big chunk establishes condition metadata. Looks thorough—just ensure coverage in tests to avoid midnight migraines.
284-284: DiscCondKey type.
This is a simple, self-explanatory export.libs/common/util/src/lib/object.ts (1)
65-87: Overloaded 'objFilter' with partial records.
That’s a welcome extension. Putting my droopy eyes to rest—this logic is well-structured.
Describe your changes
Issue or discord link
Testing/validation
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
New Features
Style