Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

468 show captured pieces instead of material imbalance #1142

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
+177 −55
Open

468 show captured pieces instead of material imbalance #1142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
afd2afc
WIP
Jimima Nov 8, 2024
ae16f79
WIP
Jimima Nov 8, 2024
f467822
Format fix
Jimima Nov 8, 2024
7691710
WIP with settings option
Jimima Nov 11, 2024
9f528bf
Version with single option with 3 choices
Jimima Nov 12, 2024
985c69f
Added in-game preference option
Jimima Nov 14, 2024
c6ff78b
Fix format
Jimima Nov 14, 2024
338fda4
Merge branch 'main' into 468-show-captured-pieces-instead-of-material…
Jimima Nov 14, 2024
f6f7f81
Removed comments
Jimima Nov 15, 2024
3fe1a77
Refactored enum
Jimima Nov 15, 2024
21fa1e9
Added test for captured pieces
Jimima Nov 15, 2024
c5e81ee
Format
Jimima Nov 15, 2024
3dfd6b3
Parameterised starting position
Jimima Nov 15, 2024
913d4a1
Refactor
Jimima Nov 15, 2024
b784f93
Removed comment
Jimima Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 36 additions & 3 deletions lib/src/model/game/material_diff.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ class MaterialDiffSide with _$MaterialDiffSide {
const factory MaterialDiffSide({
required IMap<Role, int> pieces,
required int score,
required IMap<Role, int> capturedPieces,
}) = _MaterialDiffSide;
}

Expand All @@ -30,11 +31,35 @@ class MaterialDiff with _$MaterialDiff {
required MaterialDiffSide white,
}) = _MaterialDiff;

factory MaterialDiff.fromBoard(Board board) {
factory MaterialDiff.fromBoard(Board board, {Board? startingPosition}) {
int score = 0;
final IMap<Role, int> blackCount = board.materialCount(Side.black);
final IMap<Role, int> whiteCount = board.materialCount(Side.white);

final IMap<Role, int> blackStartingCount =
startingPosition?.materialCount(Side.black) ??
Board.standard.materialCount(Side.black);
final IMap<Role, int> whiteStartingCount =
startingPosition?.materialCount(Side.white) ??
Board.standard.materialCount(Side.white);

IMap<Role, int> subtractPieceCounts(
IMap<Role, int> startingCount,
IMap<Role, int> subtractCount,
) {
IMap<Role, int> capturedPieces = IMap();
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be written in a pure functional way:

return startingCount.map(
    (role, count) => MapEntry(role, count - (subtractCount.get(role) ?? 0)),
);

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I thought this should be possible but I couldn't figure out the right way to do it

startingCount.forEach((role, count) {
capturedPieces =
capturedPieces.add(role, count - (subtractCount.get(role) ?? 0));
});
return capturedPieces;
}

final IMap<Role, int> blackCapturedPieces =
subtractPieceCounts(whiteStartingCount, whiteCount);
final IMap<Role, int> whiteCapturedPieces =
subtractPieceCounts(blackStartingCount, blackCount);

Map<Role, int> count;
Map<Role, int> black;
Map<Role, int> white;
Expand Down Expand Up @@ -80,8 +105,16 @@ class MaterialDiff with _$MaterialDiff {
});

return MaterialDiff(
black: MaterialDiffSide(pieces: black.toIMap(), score: -score),
white: MaterialDiffSide(pieces: white.toIMap(), score: score),
black: MaterialDiffSide(
pieces: black.toIMap(),
score: -score,
capturedPieces: blackCapturedPieces,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you extend test/model/game/material_diff_test.dart with a check for capturedPieces?

),
white: MaterialDiffSide(
pieces: white.toIMap(),
score: score,
capturedPieces: whiteCapturedPieces,
),
);
}

Expand Down
25 changes: 21 additions & 4 deletions lib/src/model/settings/board_preferences.dart
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

There's some commented-out code here that probably needs to be removed

Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import 'package:chessground/chessground.dart';
import 'package:flutter/material.dart';
import 'package:flutter/widgets.dart';
import 'package:freezed_annotation/freezed_annotation.dart';
import 'package:lichess_mobile/src/model/settings/preferences_storage.dart';
Expand Down Expand Up @@ -80,9 +81,11 @@ class BoardPreferences extends _$BoardPreferences
);
}

Future<void> toggleShowMaterialDifference() {
Future<void> setMaterialDifferenceFormat(
MaterialDifferenceFormat materialDifferenceFormat,
) {
return save(
state.copyWith(showMaterialDifference: !state.showMaterialDifference),
state.copyWith(materialDifferenceFormat: materialDifferenceFormat),
);
}

Expand Down Expand Up @@ -110,7 +113,7 @@ class BoardPrefs with _$BoardPrefs implements Serializable {
required bool boardHighlights,
required bool coordinates,
required bool pieceAnimation,
required bool showMaterialDifference,
required MaterialDifferenceFormat materialDifferenceFormat,
@JsonKey(
defaultValue: PieceShiftMethod.either,
unknownEnumValue: PieceShiftMethod.either,
Expand All @@ -137,7 +140,7 @@ class BoardPrefs with _$BoardPrefs implements Serializable {
boardHighlights: true,
coordinates: true,
pieceAnimation: true,
showMaterialDifference: true,
materialDifferenceFormat: MaterialDifferenceFormat.materialDifference,
pieceShiftMethod: PieceShiftMethod.either,
enableShapeDrawings: true,
magnifyDraggedPiece: true,
Expand Down Expand Up @@ -300,3 +303,17 @@ enum BoardTheme {
errorBuilder: (context, o, st) => const SizedBox.shrink(),
);
}

enum MaterialDifferenceFormat {
materialDifference(label: 'Material difference'),
capturedPieces(label: 'Captured pieces'),
hidden(label: 'Hidden');

const MaterialDifferenceFormat({
required this.label,
});

final String label;
Copy link
Contributor

Choose a reason for hiding this comment

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

The label will be translated at some point, so it's maybe better to already make this a method along the lines of String l10n(BuildContext context) => switch (this) { ... } instead (and add a //TODO l10n)

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I am not 100% sure what you mean here (though I understand the broader point). Is there an example somewhere else I could look at?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, I meant something like this:

mobile/lib/src/model/study/study_filter.dart

Line 16 in ca78193

String l10n(AppLocalizations l10n) => switch (this) {

But actually it's more common in the codebase to do it via a free function, like this:

mobile/lib/src/view/settings/theme_screen.dart

Line 31 in ca78193

String shapeColorL10n(

Copy link
Author

Choose a reason for hiding this comment

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

Hmm yeah I think maybe it sitting in the enum itself makes more sense but I'm not sure. I can implement either approach if there is an opinion either way. For now I might just go with option A, it's easy to change


bool get visible => this != MaterialDifferenceFormat.hidden;
}
10 changes: 6 additions & 4 deletions lib/src/view/correspondence/offline_correspondence_game_screen.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,9 @@ class _BodyState extends ConsumerState<_Body> {

@override
Widget build(BuildContext context) {
final shouldShowMaterialDiff = ref.watch(
final materialDifference = ref.watch(
boardPreferencesProvider.select(
(prefs) => prefs.showMaterialDifference,
(prefs) => prefs.materialDifferenceFormat,
),
);

Expand All @@ -157,9 +157,10 @@ class _BodyState extends ConsumerState<_Body> {

final black = GamePlayer(
player: game.black,
materialDiff: shouldShowMaterialDiff
materialDiff: materialDifference.visible
? game.materialDiffAt(stepCursor, Side.black)
: null,
materialDifferenceFormat: materialDifference,
shouldLinkToUserProfile: false,
mePlaying: youAre == Side.black,
confirmMoveCallbacks: youAre == Side.black && moveToConfirm != null
Expand All @@ -179,9 +180,10 @@ class _BodyState extends ConsumerState<_Body> {
);
final white = GamePlayer(
player: game.white,
materialDiff: shouldShowMaterialDiff
materialDiff: materialDifference.visible
? game.materialDiffAt(stepCursor, Side.white)
: null,
materialDifferenceFormat: materialDifference,
shouldLinkToUserProfile: false,
mePlaying: youAre == Side.white,
confirmMoveCallbacks: youAre == Side.white && moveToConfirm != null
Expand Down
6 changes: 4 additions & 2 deletions lib/src/view/game/game_body.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,10 @@ class GameBody extends ConsumerWidget {

final black = GamePlayer(
player: gameState.game.black,
materialDiff: boardPreferences.showMaterialDifference
materialDiff: boardPreferences.materialDifferenceFormat.visible
? gameState.game.materialDiffAt(gameState.stepCursor, Side.black)
: null,
materialDifferenceFormat: boardPreferences.materialDifferenceFormat,
timeToMove: gameState.game.sideToMove == Side.black
? gameState.timeToMove
: null,
Expand Down Expand Up @@ -168,9 +169,10 @@ class GameBody extends ConsumerWidget {
);
final white = GamePlayer(
player: gameState.game.white,
materialDiff: boardPreferences.showMaterialDifference
materialDiff: boardPreferences.materialDifferenceFormat.visible
? gameState.game.materialDiffAt(gameState.stepCursor, Side.white)
: null,
materialDifferenceFormat: boardPreferences.materialDifferenceFormat,
timeToMove: gameState.game.sideToMove == Side.white
? gameState.timeToMove
: null,
Expand Down
68 changes: 47 additions & 21 deletions lib/src/view/game/game_player.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ import 'dart:async';

import 'package:cached_network_image/cached_network_image.dart';
import 'package:dartchess/dartchess.dart';
import 'package:fast_immutable_collections/fast_immutable_collections.dart';
import 'package:flutter/cupertino.dart';
import 'package:flutter/material.dart';
import 'package:lichess_mobile/src/constants.dart';
import 'package:lichess_mobile/src/model/game/material_diff.dart';
import 'package:lichess_mobile/src/model/game/player.dart';
import 'package:lichess_mobile/src/model/settings/board_preferences.dart';
import 'package:lichess_mobile/src/styles/lichess_colors.dart';
import 'package:lichess_mobile/src/styles/lichess_icons.dart';
import 'package:lichess_mobile/src/styles/styles.dart';
Expand All @@ -25,6 +27,7 @@ class GamePlayer extends StatelessWidget {
required this.player,
this.clock,
this.materialDiff,
this.materialDifferenceFormat,
this.confirmMoveCallbacks,
this.timeToMove,
this.shouldLinkToUserProfile = true,
Expand All @@ -36,6 +39,7 @@ class GamePlayer extends StatelessWidget {
final Player player;
final Widget? clock;
final MaterialDiffSide? materialDiff;
final MaterialDifferenceFormat? materialDifferenceFormat;

/// if confirm move preference is enabled, used to display confirmation buttons
final ({VoidCallback confirm, VoidCallback cancel})? confirmMoveCallbacks;
Expand Down Expand Up @@ -140,27 +144,11 @@ class GamePlayer extends StatelessWidget {
),
if (timeToMove != null)
MoveExpiration(timeToMove: timeToMove!, mePlaying: mePlaying)
else if (materialDiff != null)
Row(
children: [
for (final role in Role.values)
for (int i = 0; i < materialDiff!.pieces[role]!; i++)
Icon(
_iconByRole[role],
size: 13,
color: Colors.grey,
),
const SizedBox(width: 3),
Text(
style: const TextStyle(
fontSize: 13,
color: Colors.grey,
),
materialDiff != null && materialDiff!.score > 0
? '+${materialDiff!.score}'
: '',
),
],
else if (materialDiff != null &&
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if both these conditions should be moved into the new MaterialDifferenceDisplay's build() method as well (returning SizedBox.shrink() if either of them is false) . This way we'd have all the logic in one place. (it would also make the ternary in lib/src/view/game/game_body.dart l. 131 redundant)

materialDifferenceFormat?.visible == true)
MaterialDifferenceDisplay(
materialDiff: materialDiff!,
materialDifferenceFormat: materialDifferenceFormat!,
)
else
// to avoid shifts use an empty text widget
Expand Down Expand Up @@ -325,6 +313,44 @@ class _MoveExpirationState extends State<MoveExpiration> {
}
}

class MaterialDifferenceDisplay extends StatelessWidget {
const MaterialDifferenceDisplay({
required this.materialDiff,
this.materialDifferenceFormat = MaterialDifferenceFormat.materialDifference,
});

final MaterialDiffSide materialDiff;
final MaterialDifferenceFormat materialDifferenceFormat;

@override
Widget build(BuildContext context) {
final IMap<Role, int> piecesToRender =
(materialDifferenceFormat == MaterialDifferenceFormat.capturedPieces
? materialDiff.capturedPieces
: materialDiff.pieces);

return Row(
children: [
for (final role in Role.values)
for (int i = 0; i < piecesToRender[role]!; i++)
Icon(
_iconByRole[role],
size: 13,
color: Colors.grey,
),
const SizedBox(width: 3),
Text(
style: const TextStyle(
fontSize: 13,
color: Colors.grey,
),
materialDiff.score > 0 ? '+${materialDiff.score}' : '',
),
],
);
}
}

const Map<Role, IconData> _iconByRole = {
Role.king: LichessIcons.chess_king,
Role.queen: LichessIcons.chess_queen,
Expand Down
29 changes: 18 additions & 11 deletions lib/src/view/game/game_settings.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import 'package:lichess_mobile/src/utils/l10n_context.dart';
import 'package:lichess_mobile/src/widgets/adaptive_bottom_sheet.dart';
import 'package:lichess_mobile/src/widgets/settings.dart';

import '../../widgets/adaptive_choice_picker.dart';
import 'game_screen_providers.dart';

class GameSettings extends ConsumerWidget {
Expand Down Expand Up @@ -109,17 +110,6 @@ class GameSettings extends ConsumerWidget {
ref.read(boardPreferencesProvider.notifier).togglePieceAnimation();
},
),
SwitchSettingTile(
title: Text(
context.l10n.preferencesMaterialDifference,
),
value: boardPrefs.showMaterialDifference,
onChanged: (value) {
ref
.read(boardPreferencesProvider.notifier)
.toggleShowMaterialDifference();
},
),
SwitchSettingTile(
title: Text(
context.l10n.toggleTheChat,
Expand All @@ -137,6 +127,23 @@ class GameSettings extends ConsumerWidget {
ref.read(gamePreferencesProvider.notifier).toggleBlindfoldMode();
},
),
SettingsListTile(
settingsLabel: const Text('Material'), //TODO: l10n
settingsValue: boardPrefs.materialDifferenceFormat.label,
onTap: () {
showChoicePicker(
context,
choices: MaterialDifferenceFormat.values,
selectedItem: boardPrefs.materialDifferenceFormat,
labelBuilder: (t) => Text(t.label),
onSelectedItemChanged: (MaterialDifferenceFormat? value) => ref
.read(boardPreferencesProvider.notifier)
.setMaterialDifferenceFormat(
value ?? MaterialDifferenceFormat.materialDifference,
),
);
},
),
],
);
}
Expand Down
3 changes: 2 additions & 1 deletion lib/src/view/over_the_board/over_the_board_screen.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,9 +301,10 @@ class _Player extends ConsumerWidget {
name: side.name.capitalize(),
),
),
materialDiff: boardPreferences.showMaterialDifference
materialDiff: boardPreferences.materialDifferenceFormat.visible
? gameState.currentMaterialDiff(side)
: null,
materialDifferenceFormat: boardPreferences.materialDifferenceFormat,
shouldLinkToUserProfile: false,
clock: clock.timeIncrement.isInfinite
? null
Expand Down
26 changes: 17 additions & 9 deletions lib/src/view/settings/board_settings_screen.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,15 +184,23 @@ class _Body extends ConsumerWidget {
.togglePieceAnimation();
},
),
SwitchSettingTile(
title: Text(
context.l10n.preferencesMaterialDifference,
),
value: boardPrefs.showMaterialDifference,
onChanged: (value) {
ref
.read(boardPreferencesProvider.notifier)
.toggleShowMaterialDifference();
SettingsListTile(
settingsLabel: const Text('Material'),
settingsValue: boardPrefs.materialDifferenceFormat.label,
onTap: () {
showChoicePicker(
context,
choices: MaterialDifferenceFormat.values,
selectedItem: boardPrefs.materialDifferenceFormat,
labelBuilder: (t) => Text(t.label),
onSelectedItemChanged: (MaterialDifferenceFormat? value) =>
ref
.read(boardPreferencesProvider.notifier)
.setMaterialDifferenceFormat(
value ??
MaterialDifferenceFormat.materialDifference,
),
);
},
),
],
Expand Down
Loading