diff --git a/lib/src/model/engine/engine.dart b/lib/src/model/engine/engine.dart index 112633d1c..47414c71f 100644 --- a/lib/src/model/engine/engine.dart +++ b/lib/src/model/engine/engine.dart @@ -115,11 +115,7 @@ class StockfishEngine implements Engine { await _stockfish.quit(); if (isDisposed) return; - await _stockfish.start( - flavor: flavor, - smallNetPath: _smallNetPath, - bigNetPath: _bigNetPath, - ); + await _stockfish.start(flavor: flavor, smallNetPath: _smallNetPath, bigNetPath: _bigNetPath); if (isDisposed) return; _stdoutSubscription = _stockfish.stdout.listen(_protocol.received); diff --git a/lib/src/model/engine/evaluation_mixin.dart b/lib/src/model/engine/evaluation_mixin.dart index f9f460847..171c05d12 100644 --- a/lib/src/model/engine/evaluation_mixin.dart +++ b/lib/src/model/engine/evaluation_mixin.dart @@ -122,8 +122,6 @@ mixin EngineEvaluationMixin on AnyNotifier on AnyNotifier on AnyNotifier on AnyNotifier _startEngineEval({bool goDeeper = false, bool forceRestart = false}) async { final curState = state.requireValue; if (!curState.isEngineAvailable(evaluationPrefs)) return; - await _evaluationService.ensureEngineInitialized( - state.requireValue.evaluationContext, - initOptions: evaluationPrefs.evaluationOptions, + + final context = curState.evaluationContext; + final prefs = evaluationPrefs; + + final work = Work( + enginePref: prefs.enginePref, + variant: context.variant, + threads: prefs.numEngineCores, + path: curState.currentPath, + searchTime: prefs.engineSearchTime, + multiPv: prefs.numEvalLines, + threatMode: false, + initialPosition: context.initialPosition, + steps: positionTree.branchesOn(curState.currentPath).map(Step.fromNode).toIList(), ); - _evaluationService - .start( - curState.currentPath, - positionTree.branchesOn(curState.currentPath).map(Step.fromNode), - initialPositionEval: positionTree.eval, - shouldEmit: _shouldEmit, - goDeeper: goDeeper, - forceRestart: forceRestart, - threatMode: curState.engineInThreatMode, - ) - ?.forEach((event) { - if (curState.engineInThreatMode) { + + final evalStream = await _evaluationService.evaluate( + work, + shouldEmit: _shouldEmit, + goDeeper: goDeeper, + forceRestart: forceRestart, + threatMode: curState.engineInThreatMode, + ); + + evalStream?.forEach((event) { + if (curState.engineInThreatMode) { + return; + } + final (work, eval) = event; + bool isSameEvalString = true; + positionTree.updateAt(work.path, (node) { + final nodeEval = node.eval; + if (nodeEval is CloudEval) { + if (nodeEval.depth >= eval.depth && + work.isDeeper != true && + work.searchTime != kMaxEngineSearchTime) { + final targetTime = work.searchTime; + final searchTime = eval.searchTime; + final likelyNodes = + ((targetTime.inMilliseconds * eval.nodes) / searchTime.inMilliseconds).round(); + // if the cloud eval is likely better, stop the local engine + // nps varies with positional complexity so this is rough, but save planet earth + if (likelyNodes < nodeEval.nodes) { + _evaluationService.stop(); + } return; } - final (work, eval) = event; - bool isSameEvalString = true; - positionTree.updateAt(work.path, (node) { - final nodeEval = node.eval; - if (nodeEval is CloudEval) { - if (nodeEval.depth >= eval.depth && - work.isDeeper != true && - work.searchTime != kMaxEngineSearchTime) { - final targetTime = work.searchTime; - final searchTime = eval.searchTime; - final likelyNodes = - ((targetTime.inMilliseconds * eval.nodes) / searchTime.inMilliseconds).round(); - // if the cloud eval is likely better, stop the local engine - // nps varies with positional complexity so this is rough, but save planet earth - if (likelyNodes < nodeEval.nodes) { - _evaluationService.stop(); - } - return; - } - } else if (nodeEval is LocalEval) { - if (nodeEval.isBetter(eval)) { - return; - } - } - isSameEvalString = eval.evalString == nodeEval?.evalString; - node.eval = eval; - }); - - if (!ref.mounted) return; - - if (work.path == state.requireValue.currentPath) { - onCurrentPathEvalChanged(isSameEvalString); + } else if (nodeEval is LocalEval) { + if (nodeEval.isBetter(eval)) { + return; } - }); + } + isSameEvalString = eval.evalString == nodeEval?.evalString; + node.eval = eval; + }); + + if (!ref.mounted) return; + + if (work.path == state.requireValue.currentPath) { + onCurrentPathEvalChanged(isSameEvalString); + } + }); } bool _shouldEmit(Work work) { diff --git a/lib/src/model/engine/evaluation_preferences.dart b/lib/src/model/engine/evaluation_preferences.dart index 2d4e11d40..fcd7a8020 100644 --- a/lib/src/model/engine/evaluation_preferences.dart +++ b/lib/src/model/engine/evaluation_preferences.dart @@ -100,13 +100,6 @@ sealed class EngineEvaluationPrefState with _$EngineEvaluationPrefState implemen factory EngineEvaluationPrefState.fromJson(Map json) { return _$EngineEvaluationPrefStateFromJson(json); } - - EvaluationOptions get evaluationOptions => EvaluationOptions( - multiPv: numEvalLines, - cores: numEngineCores, - searchTime: engineSearchTime, - enginePref: enginePref, - ); } Duration _searchTimeDefault() { diff --git a/lib/src/model/engine/evaluation_service.dart b/lib/src/model/engine/evaluation_service.dart index 96e841c93..62e75d213 100644 --- a/lib/src/model/engine/evaluation_service.dart +++ b/lib/src/model/engine/evaluation_service.dart @@ -6,7 +6,6 @@ import 'dart:math'; import 'package:connectivity_plus/connectivity_plus.dart'; import 'package:crypto/crypto.dart'; import 'package:dartchess/dartchess.dart' hide File; -import 'package:fast_immutable_collections/fast_immutable_collections.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart' show AlertDialog, Navigator, Text, showAdaptiveDialog; import 'package:flutter_riverpod/flutter_riverpod.dart'; @@ -14,7 +13,6 @@ import 'package:freezed_annotation/freezed_annotation.dart'; import 'package:lichess_mobile/src/model/common/chess.dart'; import 'package:lichess_mobile/src/model/common/eval.dart'; import 'package:lichess_mobile/src/model/common/preloaded_data.dart'; -import 'package:lichess_mobile/src/model/common/uci.dart'; import 'package:lichess_mobile/src/model/engine/engine.dart'; import 'package:lichess_mobile/src/model/engine/evaluation_preferences.dart'; import 'package:lichess_mobile/src/model/engine/work.dart'; @@ -67,7 +65,8 @@ class EvaluationService { Engine? _engine; bool _engineInitInProgress = false; - EvaluationContext? _context; + /// The engine preference the current engine was initialized with. + ChessEnginePref? _currentEnginePref; final ValueNotifier _nnueDownloadProgress = ValueNotifier(0.0); bool _nnueOperationInProgress = false; @@ -76,13 +75,6 @@ class EvaluationService { bool get isDownloadingNNUEFiles => nnueDownloadProgress.value > 0.0 && nnueDownloadProgress.value < 1.0; - EvaluationOptions options = EvaluationOptions( - enginePref: ChessEnginePref.sf16, - multiPv: 1, - cores: defaultEngineCores, - searchTime: const Duration(seconds: 10), - ); - static const _defaultState = ( engineName: 'Stockfish', state: EngineState.initial, @@ -120,24 +112,19 @@ class EvaluationService { } } - /// Initialize the engine with the given context and options. + /// Initialize the engine with the given preference. /// /// If the engine is already initialized, it is disposed first. - /// - /// If [options] is not provided, the default options are used. - /// This method must be called before calling [start]. It is the caller's - /// responsibility to close the engine. - Future _initEngine(EvaluationContext context, {EvaluationOptions? initOptions}) async { + Future _initEngine(ChessEnginePref enginePref) async { disposeEngine(); - _context = context; - if (initOptions != null) options = initOptions; - ChessEnginePref pref = options.enginePref; - if (options.enginePref == ChessEnginePref.sfLatest) { + ChessEnginePref pref = enginePref; + if (enginePref == ChessEnginePref.sfLatest) { if (!await checkNNUEFiles()) { _logger.warning('NNUE files not found or corrupted. Falling back to SF16.'); pref = ChessEnginePref.sf16; } } + _currentEnginePref = pref; _engine = _engineFactory(pref); _engine!.state.addListener(() { _logger.fine('Engine state: ${_engine?.state.value}'); @@ -155,23 +142,19 @@ class EvaluationService { }); } - /// Ensure the engine is initialized with the given context and options. - Future ensureEngineInitialized( - EvaluationContext context, { - EvaluationOptions? initOptions, - }) async { + /// Ensure the engine is initialized with the given preference. + /// + /// The engine is re-initialized if the preference has changed. + Future _ensureEngineInitialized(ChessEnginePref enginePref) async { if (_engineInitInProgress) { _logger.warning('Engine initialization already in progress, ignoring request'); return; } - if (_engine == null || - _engine?.isDisposed == true || - _context != context || - options != initOptions) { + if (_engine == null || _engine?.isDisposed == true || _currentEnginePref != enginePref) { _engineInitInProgress = true; try { - await _initEngine(context, initOptions: initOptions); + await _initEngine(enginePref); } finally { _engineInitInProgress = false; } @@ -201,33 +184,37 @@ class EvaluationService { _state.dispose(); } - /// Start the engine evaluation with the given [path] and [steps]. + /// Evaluate a position using the engine. + /// + /// Takes a [Work] object containing all the parameters for the evaluation. /// /// Returns a stream of [EvalResult]s. The stream is throttled to emit at most /// one value every 200 milliseconds. /// For each evaluation in the stream, if [shouldEmit] returns true, the eval /// is emitted by the [EngineEvaluation] provider. /// - /// [initEngine] must be called before calling this method. - Stream? start( - UciPath path, - Iterable steps, { - ClientEval? initialPositionEval, + /// The engine is automatically initialized if needed. + /// + /// If [goDeeper] is true, the search time is overridden to [kMaxEngineSearchTime]. + /// If [threatMode] is true, the work is modified to evaluate threats. + Future?> evaluate( + Work work, { required ShouldEmitEvalFilter shouldEmit, bool goDeeper = false, bool threatMode = false, /// If true, forces the engine to restart the evaluation even if the saved eval has more search time. bool forceRestart = false, - }) { - final context = _context; - final engine = _engine; - if (context == null || engine == null) { - assert(false, 'Engine not initialized'); + }) async { + if (!engineSupportedVariants.contains(work.variant)) { return null; } - if (!engineSupportedVariants.contains(context.variant)) { + await _ensureEngineInitialized(work.enginePref); + + final engine = _engine; + if (engine == null) { + assert(false, 'Engine not initialized'); return null; } @@ -239,23 +226,17 @@ class EvaluationService { currentWork: null, ); - final work = Work( - variant: context.variant, - threads: options.cores, + final effectiveWork = work.copyWith( hashSize: maxMemory, threatMode: threatMode, - searchTime: goDeeper ? kMaxEngineSearchTime : options.searchTime, + searchTime: goDeeper ? kMaxEngineSearchTime : work.searchTime, isDeeper: goDeeper, - multiPv: options.multiPv, - path: path, - initialPosition: context.initialPosition, - steps: IList(steps), ); - if (!work.threatMode && !forceRestart) { - switch (work.evalCache) { + if (!effectiveWork.threatMode && !forceRestart) { + switch (effectiveWork.evalCache) { // if the search time is greater than the current search time, don't evaluate again - case final LocalEval localEval when localEval.searchTime >= work.searchTime: + case final LocalEval localEval when localEval.searchTime >= effectiveWork.searchTime: case CloudEval _ when goDeeper == false: // stop the engine if running (can happen if last eval was launched with goDeeper = true) engine.stop(); @@ -266,7 +247,7 @@ class EvaluationService { } final evalStream = engine - .start(work) + .start(effectiveWork) .throttle(kEngineEvalEmissionThrottleDelay, trailing: true); evalStream.forEach((t) { @@ -450,16 +431,6 @@ sealed class EvaluationContext with _$EvaluationContext { _EvaluationContext; } -@freezed -sealed class EvaluationOptions with _$EvaluationOptions { - const factory EvaluationOptions({ - required ChessEnginePref enginePref, - required int multiPv, - required int cores, - required Duration searchTime, - }) = _EvaluationOptions; -} - /// A function to choose the eval that should be displayed. Eval? pickBestEval({ /// The eval from the local engine diff --git a/lib/src/model/engine/work.dart b/lib/src/model/engine/work.dart index fbf320538..62e730854 100644 --- a/lib/src/model/engine/work.dart +++ b/lib/src/model/engine/work.dart @@ -5,6 +5,7 @@ import 'package:lichess_mobile/src/model/common/chess.dart'; import 'package:lichess_mobile/src/model/common/eval.dart'; import 'package:lichess_mobile/src/model/common/node.dart'; import 'package:lichess_mobile/src/model/common/uci.dart'; +import 'package:lichess_mobile/src/model/engine/evaluation_preferences.dart'; part 'work.freezed.dart'; @@ -16,6 +17,7 @@ sealed class Work with _$Work { const Work._(); const factory Work({ + required ChessEnginePref enginePref, required Variant variant, required int threads, int? hashSize, diff --git a/test/model/engine/engine_test.dart b/test/model/engine/engine_test.dart index 0e4713828..8b2186368 100644 --- a/test/model/engine/engine_test.dart +++ b/test/model/engine/engine_test.dart @@ -5,6 +5,7 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:lichess_mobile/src/model/common/chess.dart'; import 'package:lichess_mobile/src/model/common/uci.dart'; import 'package:lichess_mobile/src/model/engine/engine.dart'; +import 'package:lichess_mobile/src/model/engine/evaluation_preferences.dart'; import 'package:lichess_mobile/src/model/engine/work.dart'; import 'package:multistockfish/multistockfish.dart'; @@ -24,6 +25,7 @@ void main() { final stockfishEngine = StockfishEngine(StockfishFlavor.variant); final work = Work( + enginePref: ChessEnginePref.sf16, variant: Variant.standard, threads: 1, path: UciPath.empty, @@ -47,6 +49,7 @@ void main() { final stockfishEngine = StockfishEngine(StockfishFlavor.variant); final work = Work( + enginePref: ChessEnginePref.sf16, variant: Variant.standard, threads: 1, path: UciPath.empty, diff --git a/test/model/engine/evaluation_service_test.dart b/test/model/engine/evaluation_service_test.dart index e6fa93d8e..a9a51fff5 100644 --- a/test/model/engine/evaluation_service_test.dart +++ b/test/model/engine/evaluation_service_test.dart @@ -1,8 +1,11 @@ import 'package:dartchess/dartchess.dart'; +import 'package:fast_immutable_collections/fast_immutable_collections.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:lichess_mobile/src/model/common/chess.dart'; +import 'package:lichess_mobile/src/model/common/uci.dart'; import 'package:lichess_mobile/src/model/engine/evaluation_preferences.dart'; import 'package:lichess_mobile/src/model/engine/evaluation_service.dart'; +import 'package:lichess_mobile/src/model/engine/work.dart'; import '../../test_container.dart'; @@ -61,69 +64,69 @@ void main() { expect(secondResult, isA()); }); - test('Concurrent engine initialization operations are prevented', () async { + test('Concurrent evaluate calls with engine initialization are handled', () async { final container = await makeContainer(); final service = container.read(evaluationServiceProvider); - const context = EvaluationContext(variant: Variant.standard, initialPosition: Chess.initial); - - const options = EvaluationOptions( + const work = Work( enginePref: ChessEnginePref.sf16, - multiPv: 1, - cores: 1, + variant: Variant.standard, + threads: 1, + path: UciPath.empty, searchTime: Duration(seconds: 1), + multiPv: 1, + threatMode: false, + initialPosition: Chess.initial, + steps: IListConst([]), ); - // Start first initialization - final firstInit = service.ensureEngineInitialized(context, initOptions: options); + // Start first evaluate (will initialize engine) + final firstEval = service.evaluate(work, shouldEmit: (_) => true); - // Immediately start second initialization while first is in progress - final secondInit = service.ensureEngineInitialized( - context, - initOptions: options.copyWith(multiPv: 2), - ); + // Immediately start second evaluate while first is initializing + final secondEval = service.evaluate(work, shouldEmit: (_) => true); - await Future.wait([firstInit, secondInit]); - - // Second call returns immediately without re-initializing. Both should complete successfully. - expect(firstInit, completes); - expect(secondInit, completes); - - // Options should still match the first initialization - expect(service.options, options); + // Both should complete without errors + await Future.wait([firstEval, secondEval]); service.disposeEngine(); }); - test('Sequential engine initializations are allowed', () async { + test('Sequential evaluations with same enginePref reuse engine', () async { final container = await makeContainer(); final service = container.read(evaluationServiceProvider); - const context = EvaluationContext(variant: Variant.standard, initialPosition: Chess.initial); - - const options = EvaluationOptions( + const work1 = Work( enginePref: ChessEnginePref.sf16, - multiPv: 1, - cores: 1, + variant: Variant.standard, + threads: 1, + path: UciPath.empty, searchTime: Duration(seconds: 1), + multiPv: 1, + threatMode: false, + initialPosition: Chess.initial, + steps: IListConst([]), ); - // First initialization - await service.ensureEngineInitialized(context, initOptions: options); + // First evaluate + await service.evaluate(work1, shouldEmit: (_) => true); - // Wait for first to complete, then start second with different options - const options2 = EvaluationOptions( + // Second evaluate with same enginePref but different params + const work2 = Work( enginePref: ChessEnginePref.sf16, - multiPv: 2, - cores: 1, + variant: Variant.standard, + threads: 2, + path: UciPath.empty, searchTime: Duration(seconds: 2), + multiPv: 2, + threatMode: false, + initialPosition: Chess.initial, + steps: IListConst([]), ); - await service.ensureEngineInitialized(context, initOptions: options2); - - // Second call should be allowed since first completed - expect(service.options, options2); + await service.evaluate(work2, shouldEmit: (_) => true); + // Both should complete without errors (engine is reused) service.disposeEngine(); }); });