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.

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

Implement proper operator error handling #343

Closed
martinRenou opened this issue Mar 29, 2024 · 3 comments · Fixed by #349
Closed

Implement proper operator error handling #343

martinRenou opened this issue Mar 29, 2024 · 3 comments · Fixed by #349
Labels
enhancement New feature or request
Milestone

Comments

@martinRenou
Copy link
Collaborator

#340 introduced new operators that may fail to compute eventually.

We should implement proper operator error handling and reporting, so that users understand what happens upon failure.

@martinRenou martinRenou added the enhancement New feature or request label Mar 29, 2024
@martinRenou
Copy link
Collaborator Author

martinRenou commented Jun 3, 2024

cc. @trungleduc

Following an internal discussion with @SylvainCorlay.

My PR #349 to handle error messages from Open Cascade is not a good approach as we do in order:

  1. Let the user confirm the operator form
  2. Apply the transaction to the shared model
  3. React to the shared model change by sending the operation to the OCC worker
  4. (New in the PR) show an error dialog upon failure
    I believe this is not a good approach because the shared model has been changed with a broken operator. This means the error dialog will show up a lot for the same error (when refreshing, when updating an operand etc).

Proposed solution

Another way to tackle this Open Cascade failure would be to make a dry run in the OCC worker to try to apply the operator before either step 1. (prevent form confirmation upon failure) or 2. (prevent shared model change upon failure).

Upon success, our worker is already smart enough to not compute the operator a second time by caching the operator output during the dry run, so performance wide this solution is good.

Right now the 3D view owns the OCC worker, AFAIK the operators commands don't have a pointer to it. So there may be some refactor do it here to make it right.

@trungleduc
Copy link
Member

Right now the 3D view owns the OCC worker, AFAIK the operators commands don't have a pointer to it. So there may be some refactor do it here to make it right.

No, the OCC worker is accessible anywhere via the IJCadWorkerRegistry token

@martinRenou
Copy link
Collaborator Author

Nice. I'll have a look at implementing this approach.

@martinRenou martinRenou added this to the 2.0.0 milestone Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants