Re #211 Centipawn graph

[limitedAtonement]

See the Sourceforge Issue related to this pull request: https://sourceforge.net/p/chessx/feature-requests/211/

[limitedAtonement]

This should not be gameTimeDock, but centipawnDock.

[limitedAtonement]

No need to setObjectName, I think. At least change the name to CentipawnDock or so.

[IAHM-COL]

@limitedAtonement @Isarhamster

Hi @limitedAtonement

I think this functionality is a great addition and looks terrific

I want to make a couple recommendations:

1. This Pull request is a mixed bag. That is not ideal for a proper pull request: The test I did above was done by just cherry-picking the proper commit: 6ed4f01, and leaving all other commits here unused, as they are unrelated

I would recommend to make the PR request more simple, only including the new function and I would be saying this gets my recommendation

2. Somehow, this pull request changes "Notation" by "Notationnn". notice the extra ns at the end.

To fix that should be simple, by editing line

     /* Game view */
-    DockWidgetEx* gameTextDock = new DockWidgetEx(tr("Notationnn"), this);
+    DockWidgetEx* gameTextDock = new DockWidgetEx(tr("Notation"), this);
     gameTextDock->setObjectName("GameTextDock");

on src/gui/mainwindow.cpp

[limitedAtonement]

@IAHM-COL Thanks for the review!

1. This Pull request is a mixed bag. That is not ideal for a proper pull request: The test I did above was done by just cherry-picking the proper commit: 6ed4f01, and leaving all other commits here unused, as they are unrelated

Absolutely right. Those other commits are required for me to be able to run the code. I only meant to contribute the last two commits. I have adjusted the pull request and limited it to those commits now!

2. Somehow, this pull request changes "Notation" by "Notationnn". notice the extra ns at the end.

Eek! I did that early on when I was poking around to see if I could change ANYTHING. I have fixed that in the current pull request!

[IAHM-COL]

@limitedAtonement

It works and looks good.

@Isarhamster

I think this is a nice addition to consider. I hope you would like taking a look at this,

Regards

[Isarhamster]

I finally managed to take a look into this request.
Summary: I really like the idea, but modifications are required.
Sidenote: In my latest release 1.6.2 on Mac OS and all Qt6 compilations, the engine options are broken (which is why setting Threads fails).
Modifications required:

1. In dark mode, the eval becomes invisible
2. I'd put the Start Analysis and the engine selection into the Settings Dialog
3. The chart view does not correctly serialize the screen sizes (this might have something to do with the object names being set or not - setting an object name is required for this to work)
4. I would like to have the evaluation to be continuous and only analyze the latest position or move added to the game (to make it more performant)
5. There are circumstances, where the analysis is not allowed (to disable cheating) - see MainWindow::enterNoHintMode
6. Handle cases where there are no engines available (this might turn irrelevant after implementing item 2)
7. I'm not sure why setting the Threads to 1 is required
8. I have found a few optimizations in ChartWidget::paintEvent() and ChartWidget::setValues() (mainly comparing new and old polygon to be same and only paint in case changes are detected)

[limitedAtonement]

Thank you for your detailed findings. It will probably take me many days to address these, but I'll post back here with any questions I have (I anticipate having several questions)!

[limitedAtonement]

1. In dark mode, the eval becomes invisible

Cannot reproduce:

chessx-analysis-darkmode-2024-09-17_07.12.53.mp4

What are you seeing?

2. I'd put the Start Analysis and the engine selection into the Settings Dialog

I was imitating the Analysis windows (which have the engine in the dockable widget). Although, I'm pretty sure Start Analysis shouldn't go into the Settings Dialog because that's how you start analysis and analysis shouldn't be done in the Settings dialog. (Do I understand your meaning?) I will put the engine selection into the settings dialog.

3. The chart view does not correctly serialize the screen sizes (this might have something to do with the object names being set or not - setting an object name is required for this to work)

I think you're talking about chessx remembering the size and position of the dockable widget so that when it starts, the graph is in the same place as last time? I'll look into this, but let me know if I'm on the wrong track.

4. I would like to have the evaluation to be continuous and only analyze the latest position or move added to the game (to make it more performant)

The use case I'm imagining for this is: after a player finishes a game, to begin reviewing, he can run this analysis to get a high-level overview of where he and his opponent made significant errors that affected the course of the game. "...only analyze the latest ... move added to the game" suggests a different use case, but I can't picture it.

Also "...only analyze the latest position..." why not analyze earlier positions? (I have many more questions about this, but will wait for your feedback first.)

5. There are circumstances, where the analysis is not allowed (to disable cheating) - see MainWindow::enterNoHintMode

Oh yes, I didn't handle that at all. I'll look into it and make the necessary adjustments.

6. Handle cases where there are no engines available (this might turn irrelevant after implementing item 2)

☑️

7. I'm not sure why setting the Threads to 1 is required

Using multiple threads would be a waste because we're not actually looking for the best move, only a reasonably close evaluation of the position. This can be done quite briefly with one thread and significantly less than one second of analysis.

If a computer has, say 50 logical cores, we want to be able to analyze 50 positions concurrently for, say, .2 seconds to get an acceptable graph. We do this by creating 50 instances of the analysis engine and asking each one to compute a particular position in the game for msPerMove milliseconds. If each analysis instance had more than 1 thread, we would either have to reduce the number of concurrent instances or we would be dealing with aggressive CPU contention (because there are more jobs than logical cores to do the jobs) which will significantly slow down the computation.

Does that make sense? Am I missing anything?

8. I have found a few optimizations in ChartWidget::paintEvent() and ChartWidget::setValues() (mainly comparing new and old polygon to be same and only paint in case changes are detected)

I don't think I touched any code in paintEvent, so any changes you make (as long as it still works 😄) should not affect the changes in this pull request.