feat: scale arrows in analysis based on winning chances

[tom-anders]

Notes

• Not sure if we still need the different opacities now...? The old app does not have them, but I think it looks nice. The Web UI seems to use a different opacity for the best move, and the same opacity for all other moves
• Scaling logic could probably be adjusted in the feature based on user feedback, but I think the result looks pretty close to the old app and the web UI already.

Screenshots

Current version:

This PR:

Old App for comparison (Shows different moves because the two screenshots above are from a debug build):

[ijm8710]

Call me crazy but I think opacity matters more than thickness.

The super thin elongated secondary Move arrows look out of place imo

If anything I'd prefer keeping the opacity + reducing the intensity of the scaling significantly

[tom-anders]

Again, I think it's important to be consistent with the Web UI.

Also, note that this is an extreme example, where only one move holds a draw, while the other two are loosing. I think it's nice that this is now immediately noticeable from the arrows, even if the variation display is disabled.

Here's another example, where the move 2 and 3 are only slightly worse:

[veloce]

@tom-anders can you solve the conflicts please?

I also think the opacity is not needed with the scaled arrows. Now that they are scaled by winning chance it is even confusing and not relevant anymore.
We could have used an opacity scale by winning chance too, but since the web ui uses thickness it is best to stick with it indeed.

[tom-anders]

@tom-anders can you solve the conflicts please?

Done!

I also think the opacity is not needed with the scaled arrows. Now that they are scaled by winning chance it is even confusing and not relevant anymore. We could have used an opacity scale by winning chance too, but since the web ui uses thickness it is best to stick with it indeed.

Okay, the latest version removes the opacity scaling entirely now. However, to be even more consistent with the Web UI, the best move now has a higher (0.4) opacity than all other moves (0.2):

Lichess Web UI for comparison:

Do we also want to align the exact color/opacity values here?

[veloce]

Yes let's keep the opacity difference between best move and others. Looks like there are some tests failing, can you check? thanks.

[tom-anders]

Only formatting issues, my bad. Need to get into the habit of running dart format before pushing

[veloce]

Oh sorry I haven't answered to the exact question. We can use the color/opacity difference from web UI between best move and others. It looks better like that I agree, and people accustomed to the web UI will feel more at home.

[tom-anders]

Right, I’ll check the server code and update the PR

[tom-anders]

@veloce by the way (sorry more or less off-topic), what’s the reasoning for using a different default piece set in the app compared to the Web UI? Don’t we want users to feel at home in that regard as well?

[veloce]

We don't want the app to look the same as the website. This was the opportunity to use a more modern, nicer piece set.

We'd like to eventually cover all the features of the website, but being 1:1 with the website is a non goal. The app will be different.

[tom-anders]

Colors are now aligned with the Web UI (thanks to quick help on discord):

[veloce]

Typo: should be MoveWithWinningChances with a C

Also I think here we could use a dart record instead of a new class:

typedef MoveWithWinningChances = (Move, double);

[tom-anders]

Fixed the typo, sorry about that.

Regarding the record: IMO this makes the code less readable, since in _computeBestMoveShapes you'd either write something like moves[index].$2 instead of moves[index].winningChances, or you'd need to use destructuring (var (move, winningChanges) = moves[index] (but then, why use a record in the first place instead of a class?)

[veloce]

You can also defined a record with named fields: https://dart.dev/language/records#record-fields

typedef MoveWithWinningChances = ({ Move move, double winningChances });

Records are a language construct so I'd use them instead of @freezed classes whenever possible (when you need an immutable data structure without logic). Freezed classes bring overhead and more process time with build_runner.

[tom-anders]

Ahh thanks for the clarification, didn't learn about that feature yet. Will change it accordingly