468 show captured pieces instead of material imbalance #1142
Conversation
|
UI looks fine! |
|
I added a secondary option to the settings for this. I wonder if this is the best approach, it was simple to add it this way but maybe a single choice with 3 options would be better 
Was also a bit unclear on why the theme preference implementation did something custom for the choice picker on ios, this seems to work but can take the other approach if there is a reason to do so |
|
I think it makes more sense to have one setting with 3 options. Rather than 2 separate toggle due to the fact that the settings screen keeps getting more options and to limit it best we can. Obv Veloce/toms opinion will carry more weight but I think it's best to keep that contained best they can |
|
Alternate option 
|
|
In-game option Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-11-14.at.08.55.42.mp4 |
|
Would appreciate a review @veloce @tom-anders |
|
It is better with a single setting yes. Thanks! |
| black: MaterialDiffSide( | ||
| pieces: black.toIMap(), | ||
| score: -score, | ||
| capturedPieces: blackCapturedPieces, |
There was a problem hiding this comment.
Can you extend test/model/game/material_diff_test.dart with a check for capturedPieces?
| IMap<Role, int> startingCount, | ||
| IMap<Role, int> subtractCount, | ||
| ) { | ||
| IMap<Role, int> capturedPieces = IMap(); |
There was a problem hiding this comment.
This could be written in a pure functional way:
return startingCount.map(
(role, count) => MapEntry(role, count - (subtractCount.get(role) ?? 0)),
);
There was a problem hiding this comment.
Thanks, I thought this should be possible but I couldn't figure out the right way to do it
| Board.standard.materialCount(Side.black); | ||
| final IMap<Role, int> whiteStartingCount = | ||
| Board.standard.materialCount(Side.white); | ||
| // TODO: parameterise starting position maybe so it can be passed in |
There was a problem hiding this comment.
Yes I think this is needed before merging - It's already possible to view (but not play) variants that have a different starting position (e.h. Horde). With this PR, capturedPieces will display incorrect values currently.
However, I noticed that the web UI does not display material difference at all for Horde, maybe we should do the same? @veloce wdyt?
There was a problem hiding this comment.
There's some commented-out code here that probably needs to be removed
|
|
||
| const MaterialDifference({ | ||
| required this.label, | ||
| required this.visible, |
There was a problem hiding this comment.
is the visible field really needed? A property bool get hidden => this != MaterialDifference.hidden would also work (and probably same some memory)
| required this.visible, | ||
| }); | ||
|
|
||
| final String label; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Sure, I meant something like this:
But actually it's more common in the codebase to do it via a free function, like this:
| @@ -110,7 +119,9 @@ class BoardPrefs with _$BoardPrefs implements Serializable { | |||
| required bool boardHighlights, | |||
| required bool coordinates, | |||
| required bool pieceAnimation, | |||
| required bool showMaterialDifference, | |||
| // required bool showMaterialDifference, | |||
| required MaterialDifference materialDifference, | |||
There was a problem hiding this comment.
Calling this MaterialDifferenceFormat (like in the commented line below) would be more readable I think. Currently, there's both MaterialDiff and MaterialDifference which is confusing
lib/src/view/game/game_player.dart
Outdated
| size: 13, | ||
| color: Colors.grey, | ||
| ), | ||
| if (materialDifference == MaterialDifference.capturedPieces) |
There was a problem hiding this comment.
Maybe it would be more readable to factor out this whole Row into a class MaterialDifferenceDisplay extends StatelessWidget. (Then you could also precompute the pieces to loop over in a helper variable and use that in the loop, instead of needing to write the loop twice)
|
@tom-anders I pushed up a bunch of changes, would appreciate you taking another look when you have the time 🙂 |
| : '', | ||
| ), | ||
| ], | ||
| else if (materialDiff != null && |
There was a problem hiding this comment.
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)
|
very nice, no more comments from me :) |
|
Oh should have said, I still don't really know the best label to use for the setting. I just changed it to simply "Material" just now but I've changed it a few times and still can't decide what is best. Happy to be told here! |
Thanks, will take a look at the l10n stuff tomorrow |
Had a go at this, draft at this stage as there is a bit to do but wanted feedback on the overall idea, if possible
Relates #468