Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts offline practice-mode evaluation/hints behavior to improve responsiveness and correctness of “before/after” comparisons, while consolidating evaluation logic (engine/cloud/tablebase) into a single helper.
Changes:
- Tune hint/move evaluation search times and introduce separate depth thresholds for hints vs. move evaluation.
- Add a unified
_getEvalpath that can race local engine eval, cloud eval, and optional tablebase lookup. - Improve practice-mode move evaluation when pre-move analysis is missing by triggering a quick “before” evaluation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
lib/src/model/offline_computer/offline_computer_game_controller.dart |
Updates hint/move eval thresholds and refactors evaluation logic to use a shared helper with optional tablebase lookup. |
lib/src/model/engine/evaluation_service.dart |
Adjusts eval timeout and avoids stopping a newer work item when a previous one times out. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/src/model/offline_computer/offline_computer_game_controller.dart
Outdated
Show resolved
Hide resolved
lib/src/model/offline_computer/offline_computer_game_controller.dart
Outdated
Show resolved
Hide resolved
| // print( | ||
| // 'Move eval computed: depth=${evalAfter.depth}, searchTime=${evalAfter is LocalEval ? evalAfter.searchTime : null} nodes=${evalAfter.nodes}', | ||
| // ); |
There was a problem hiding this comment.
These commented-out print(...) debug statements add noise to the production code path. Consider removing them (or gating them behind kDebugMode logging) to keep the file clean.
| // print( | |
| // 'Move eval computed: depth=${evalAfter.depth}, searchTime=${evalAfter is LocalEval ? evalAfter.searchTime : null} nodes=${evalAfter.nodes}', | |
| // ); |
| _getEvalTimer?.cancel(); | ||
| _getEvalTimer = Timer(work.searchTime + const Duration(seconds: 2), () { | ||
| _getEvalTimer = Timer(work.searchTime + const Duration(seconds: 3), () { | ||
| if (!completer.isCompleted) { | ||
| completer.complete(null); | ||
| } | ||
| }); |
There was a problem hiding this comment.
_getEvalTimer is a single shared field and is cancelled/reassigned on every _getEval call. If multiple _getEval calls overlap (e.g., hints evaluation while a practice move evaluation starts), a later call can cancel the earlier call’s fallback timer, potentially leaving that earlier completer without its safety timeout if an exception occurs. Consider making the fallback timer local to _getEval (and cancelling it when the completer completes) so concurrent calls don’t interfere with each other.
| ); | ||
| } | ||
|
|
||
| /// Gets an evaluation by requesting at the same time the local engine and a cloud eval and optionnally doing a tablebase lookup. |
There was a problem hiding this comment.
Typo in dartdoc: "optionnally" should be "optionally".
| /// Gets an evaluation by requesting at the same time the local engine and a cloud eval and optionnally doing a tablebase lookup. | |
| /// Gets an evaluation by requesting at the same time the local engine and a cloud eval and optionally doing a tablebase lookup. |
| // print( | ||
| // 'Hints computed: depth = ${finalEval?.depth}, searchTime = ${finalEval is LocalEval ? finalEval.searchTime : null} nodes=${finalEval?.nodes}', | ||
| // ); |
There was a problem hiding this comment.
These commented-out print(...) debug statements add noise to the production code path. Consider removing them (or gating them behind kDebugMode logging) to keep the file clean.
| // print( | |
| // 'Hints computed: depth = ${finalEval?.depth}, searchTime = ${finalEval is LocalEval ? finalEval.searchTime : null} nodes=${finalEval?.nodes}', | |
| // ); |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.