Skip to content

Add B50 Support - #71

Merged
283375 merged 35 commits into
masterfrom
refactor/b50
Oct 4, 2026
Merged

283375 merged 35 commits into
masterfrom
refactor/b50

Conversation

@283375

@283375 283375 commented Sep 21, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

Arcaea v7.0 reworked the potential system: B30 + R10 is replaced by B50
(top 10 entries counted twice), and single-play potential now adds a
+0.2 clear bonus for any state other than TRACK_LOST.

ArcaeaScoringMode is keyed by the date each rule took effect so the key
can be persisted as-is and stays chronologically ordered. A missing
clear type counts as TRACK_LOST: historical records without a reliable
clear state must not gain the bonus, so the default keeps B50 values
conservative rather than inflating them.
The scoring mode describes how a database's play results are interpreted
(official servers use B50 while private servers may stay on B30 + R10),
so it belongs to the database itself rather than app preferences. Storing
it in the properties table keeps it traveling with the database file and
makes a per-database scope automatic.

Unset or unknown keys fall back to the latest mode (B50).
PotentialRepository gains b50(), b10() and a scoring-mode-aware
potential(). B50 follows the official v7.0 formula (best50 sum plus
best10 sum, divided by 60, so the top 10 count twice) over the
clear-bonus-inclusive play rating; B30_R10 keeps (b30 + r10) weighting.

Best-per-chart ranking becomes scoring-mode aware: under B50 a cleared
play can outrank a higher-scoring TRACK_LOST play of the same chart, so
the per-chart best must be selected by the bonus-inclusive rating. The
minimum-fields query now carries clear_type for that comparison.

The B30 list call site passes B30_R10 explicitly; behavior is unchanged.
potentialToText now takes the decimal scale directly (default 3) instead
of a DecimalMode, which no caller customized. There is no community
consensus on potential display precision and some tools show the raw
value, so 3 decimals is used everywhere by default; the overview main
potential switches per scoring mode separately.

The chart recommend result row used a hardcoded "%.2f" format; it now
goes through potentialToText like every other ptt display.
The overview card follows the scoring mode stored in the database: B30 +
R10 shows the legacy averages with the official 0.01 precision, B50 shows
the B50/B10 averages with 0.001. The overall potential value comes from
PotentialRepository.potential() which already switches on the mode.

Also annotate MinimumPlayResultPotentialFields.clearType with
@ColumnInfo so Room maps the new query column.
The scoring mode is a property of the active database, so the switch
lives in the database manage screen instead of the app settings. Adds a
generic radio-button SelectPreferencesWidget beside the existing
preference widgets.
Under B50 the best list shows the clear-bonus-inclusive ratings and the
entry is titled Best50; the R30 entry is hidden since the recent queue
does not participate in B50. The score list also rates each record by
the active mode's rules. Behavior under B30 + R10 is unchanged.
Under the B50 rules the clear state changes the single-play potential by
the clear bonus, so the calculator takes it as an input (default: no
clear type, i.e. no bonus). The result is shown to 6 decimals as a tool
output, independent of the official display precision.
A record without a clear type is rated as TRACK LOST under the B50
rules, which is invisible to the user. The new CLEAR_TYPE_MISSING
warning surfaces such records in the play result editor and lists so
they can be filled in; it only appears under B50, where it affects the
rating.

The warning stays out of the plain validate(): OCR import uses it as a
data-quality gate, and a missing clear type is valid data there.
The initial target play rating mirrors the player's current level; use
the b50 average under B50 instead of the b30 average, which no longer
represents it.
…lator

Part of the previous calculator commit did not land: the result was
still computed without the clear type and formatted at 4 decimals, and
the clear-type row was missing so the selector dialog was unreachable.
Class names, package declaration, enum entry, route, string keys and
references now say best_list; the screen title no longer switches per
scoring mode since the entry name is mode-neutral.
Renders as a TextPreferencesWidget row: the label as title, the current
selection as the description line and a trailing expand icon, matching
the other preference-driven selectors instead of an ad-hoc text row.
@283375 283375 changed the title Add B50 support Add B50 Support Sep 21, 2026
A fresh database has no `scoring_mode` row either, so it follows the
fallback value until the user touches the mode selector. That fallback can
change in a later release, and a database nobody configured would silently
move with it.

PropertyRepository.DEFAULT_SCORING_MODE names the fallback, so the callback
that writes it and the read path that falls back to it cannot drift apart.
@opencode-agent

Copy link
Copy Markdown

Model not found: deepseek/deepseek-v4-flash. Did you mean: deepseek-flash, deepseek-v4-pro?

opencode session  |  github run

@283375

283375 commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

/opencode Review this PR

@opencode-agent

Copy link
Copy Markdown

Model not found: deepseek/deepseek-v4-flash. Did you mean: deepseek-flash, deepseek-v4-pro?

opencode session  |  github run

@283375

283375 commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

/opencode Review this PR

This reverts commit d525a9c.

# Conflicts:
#	.github/workflows/opencode.yml
@283375

283375 commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

/opencode Review this PR

@opencode-agent

Copy link
Copy Markdown

PR Review: Add B50 Support (#71)

I reviewed the full diff, then compiled shared, core, and app and ran the runnable tests. All of those pass:

  • :shared:jvmTest (new clear-bonus / calculatePlayRating overload tests)
  • :app:testStableDebugUnitTest (formatter tests)
  • :core:testDebugUnitTest (new ArcaeaSt3PlayResultImporterTest)
  • :core:compileDebugAndroidTestKotlin (new instrumented tests compile)

The instrumented tests (R30QueueUpdaterTest, PotentialRepositoryTest, migration tests) couldn't be executed here (no emulator).

Overall the feature is well-structured, documented (docs/potential.md, docs/st3.md), and backed by real-save regression tests. There are a few things to address before merge.

Blocking

  1. CI model name is broken — .github/workflows/opencode.yml:47 is set to deepseek/deepseek-v4-flash, which is not a valid model (the two review runs on this PR already failed with Model not found: deepseek/deepseek-v4-flash). Master uses ${{ vars.OPENCODE_MODEL || 'deepseek/deepseek-flash' }}. This looks like a bad conflict resolution in the Revert "ci: update deepseek model name" commit (c1873b9). This change is unrelated to B50 and should be reverted/dropped.

Medium

  1. B50 recommendation ignores the clear bonus — UtilitiesChartRecommendScreenViewModel.kt:56-57 seeds targetPlayRating from potentialRepo.b50(), which includes the +0.2 clear bonus, but the filter (:79-81) and UtilitiesChartRecommendScreen.kt:197-199 compute calculatePlayRating(score, constant) without clearType. So in B50 mode the recommended scores/displayed actualPlayRating are consistently off by 0.2 for any non-TL play. Either thread the scoring mode/clear type through the filter, or seed from the bonus-exclusive value.

  2. Divisor inconsistency between B50 values — PotentialRepository.kt:71 computes b50() as sum / entries.size, while potential() (:97-100) always divides by 60. For players with fewer than 50 best entries, the displayed B50 and the overall potential treat missing entries differently, so the card can show an inconsistent picture. (b30 = b30*0.75 + r10*0.25 has the analogous assumption vs. the documented /40.) Worth deciding on one rule.

Low / Nits

  1. R30 distinct-chart floor no longer enforced while the queue is filling — R30QueueUpdater.kt:70 returns early when entries.size < 30 without the distinctCharts >= 10 check, so unlike the old updateR30List, a duplicate-chart play can be admitted while the queue has <10 distinct charts. Probably harmless in practice, but it's a behavior change not covered by the tests.

  2. Migration sets existing databases to B50 — Migration_15_16 writes scoring_mode = 20260827 for all v15 DBs. That reinterprets pre-v7 data under B50, where previously-imported unreliable clear types are stored as null (the old isClearTypeReliable check) and thus earn no bonus. The new CLEAR_TYPE_MISSING warning surfaces it, but there's no way to backfill; consider calling this out in the migration/PR notes.

  3. clearType state is not saved — PlayRatingCalculator.kt:50 uses remember while showClearTypeSelectDialog uses rememberSaveable, so the chosen clear type is lost on rotation. Also, the "no clear type" row (index 0) never appears selected because selectedOptionIndex is null for it.

  4. ScoringModeCreateCallback.kt and Migration_15_16.kt both hardcode/duplicate the default mode; the migration comment justifies the literal, but the callback could use the constant (it already does) — just noting the two paths need to stay in sync.

Happy to expand on any of these.

New%20session%20-%202026-09-24T18%3A09%3A44.019Z
opencode session  |  github run

@283375
283375 marked this pull request as ready for review September 24, 2026 18:22
@283375

283375 commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator Author

/opencode Review the new commits since last review (be6ee71..a78d1a6)

@opencode-agent

opencode-agent Bot commented Oct 4, 2026

Copy link
Copy Markdown

Review: new commits be6ee714..a78d1a6 (9 commits)

Scope: the follow-up commits that refactor PotentialRepository into PotentialGroups, thread the clear bonus through the chart recommender, drop the incremental R30 mode, and add tests.

Verification

  • :shared:jvmTest, :app:testStableDebugUnitTest, :core:testDebugUnitTest — pass.
  • :core:compileDebugAndroidTestKotlin — compiles. Instrumented tests (PotentialRepositoryTest, R30QueueUpdaterTest) not executed here (no emulator; CI runs them).

Prior review items now resolved

  • Clear bonus in the recommender (was medium): filterChartsByTarget (UtilitiesChartRecommendScreenViewModel.kt:43-56), ResultsListItemState (UtilitiesChartRecommendScreen.kt:186-207) and the calculator now all thread the same clear type, so filter/display can't drift from each other. Good.
  • Divisor inconsistency (was medium): groups() now exposes raw total + isComplete (PotentialRepository.kt:11-22), and callers divide by the fixed official divisors (calculatePotentialB50/B30R10). No more sum/size vs /60 split. The completeness warning in OverviewPotentialCard.kt:104-133 covers partial saves.
  • clearType not saved (was low): now rememberSaveable (PlayRatingCalculator.kt:50); the "no clear type" dialog row is gone.
  • R30 fill-phase floor (was low): now explicitly documented in docs/potential.md as intended.
  • Dead code: ChartBestScore, findLaterThan, bestScoresUntil, deleteR30LastUpdatedAt, RunMode all removed cleanly; no dangling references.

Remaining / nits

  1. Seed target vs default clear type (low): UtilitiesChartRecommendScreenViewModel.kt:80-83 seeds targetPlayRating from the bonus-inclusive overall potential, but the new toggle defaults to clearType = null. Default recommendations are therefore computed bonus-free. It's conservative and self-consistent per row, but the target and candidates mean slightly different things until the user flips the switch. Consider defaulting to cleared or seeding the bonus-exclusive value.
  2. Migration_15_16 still reinterprets legacy data (low, carried over): Migration_15_16.kt:19 assigns 20260827 to every pre-v16 DB, so pre-existing plays with clear_type = null earn no bonus under B50. The CLEAR_TYPE_MISSING warning surfaces it, but there's no backfill; worth an explicit call-out in the migration/PR notes.
  3. Precision nit: the B10/B50/B30/R10 rows render the raw sum and the quotient at 4 decimals (OverviewPotentialCard.kt:67,79) while the headline uses 2/3. Probably intentional to show sum precision, but B10/B50 display precision now differs from the documented 3.
  4. Test nit: CommonCalculatorsTest.testInvertPlayRatingWithClearBonus still passes the computed value first (assertEquals(actual, expected)); the earlier "fix wrong assertEqual orders" pass didn't cover it. No behavior impact.

No new blocking issues. The functional changes are consistent, and the added regression tests are meaningful (I verified the b50PrefersTheClearedPlayOverTheHigherScoredTrackLost expectation against the seed: TL 13.1 vs cleared 13.15 in B50, reverse in B30).

New%20session%20-%202026-10-04T13%3A54%3A47.733Z
opencode session  |  github run

Comment on lines +40 to +53
fun validate(
playResult: PlayResult,
chartInfo: ChartInfo?,
scoringMode: ArcaeaScoringMode,
): List<ArcaeaPlayResultValidatorWarning> {
val clearTypeMissing =
if (
scoringMode == ArcaeaScoringMode.B50 &&
ArcaeaPlayResultValidatorClearTypeMissingWarning.conditionsMet(playResult, chartInfo)
) {
listOf(ArcaeaPlayResultValidatorClearTypeMissingWarning)
} else {
emptyList()
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Consider future refactors?

@283375
283375 merged commit c95e80d into master Oct 4, 2026
4 checks passed
@283375
283375 deleted the refactor/b50 branch October 4, 2026 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant