cutelyaware / magiccube4d

Automatically exported from code.google.com/p/magiccube4d
Other
71 stars 16 forks source link

A cancel button for puzzle building #140

Open cutelyaware opened 4 years ago

cutelyaware commented 4 years ago

With Don's support of lovely new puzzles comes a greater need to be able to cancel puzzle creation of the truly humongous ones. I was able to find a reasonable way to support this in the UI, and to make it start to work, but debugging all the possible race conditions is beyond what either of us is willing to do to finish the task. Here is the patch containing my attempt: magiccube4d-cancel.zip

Here is Don's feedback:

(1) it looks like your patch changes that inBackground param you're talking about from false to true in one or maybe two calls during startup (one from MC4DSwing.java, maybe only if there's a log file, and one from inside the PuzzleManager ctor). I don't know if that was intentional, but I think maybe those should remain false? The fact that they are now true makes it so that the puzzle creation happens in the SwingWorker thread, and doesn't complete by the time PipelineUtils needs it to draw the first frame, so that's why you're getting null stuff there. If I change them back to false, that seems to fix the exceptions on startup.

(2) You'll need to keep track of whether you cancelled the PolytopePuzzleDescription ctor, and, if so, don't use the constructed PolytopePuzzleDescription object, since it's bad. Actually, I'm thinking it would probably be simpler and less error-prone if, instead of having your updateProgress() callback return false on cancellation, just make it throw some exception of your choice instead; that'll throw all the way out of the PolytopePuzzleDescription ctor and back up into your calling code, and your catch block can handle it however you like, and there will be no bad object for you to have to go out of your way to avoid using. So, I think maybe I can change all the callbacks to return void instead of bools, that'll simply things and make this kind of hard-to-diagnose bug in the caller impossible.

(3) Even before your patch, it looked to me like the background thread creation of the puzzle is likely generally racy, as I think you've pointed out in your comments or original commit description (I forget which). In particular this:

public Void doInBackground() {
PuzzleDescription newPuzzle = buildPuzzle(schlafli, lengthString, this);
if(newPuzzle != null) {
succeeded = true;
puzzleDescription = newPuzzle;
faceColors = ColorUtils.generateVisuallyDistinctColors(puzzleDescription.nFaces(), .7f, .1f);
resetPuzzleStateNoEvent();
}
return null;
}

So it looks like the background thread is changing several pieces of state here, non-atomically, at the same time that the foreground thread (EDT) is using all that state. While it might be getting by, to some extent, by luck, I'm not surprised if it's crashy and unpredictable, and so it might be difficult to get very far with further enhancements like this Cancel button.