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

handle box errors in graphics and graphics3d #966

Merged
merged 6 commits into from
Jan 19, 2024

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Jan 17, 2024

Here we go with a proposal to address #964. The idea is that when an invalid graphics primitive is processed, a pink rectangle is drawn instead. Notice that in WMA no error message is shown in the client. Just the pink background is shown in the image, plus a tooltip in the notebook interface.

@rocky
Copy link
Member

rocky commented Jan 18, 2024

Graphics3D[{Red, Opacity[0.25], Cuboid[{{0, 0, 0}, {0.5, 0.5, 0.5}}]}]

is probably a little bit closer. Also it looks like the error message is some sort of legend or alt text for the image.

The closest approximation I can come up with in our limited set of graphics is:

Row[{Graphics3D[{Red, Opacity[0.25], Cuboid[{{0, 0, 0}, {0.5, 0.5, 0.5}}]}], "Cilinder is not a graphics primitive"}]]

An canned image would also work.

@mmatera
Copy link
Contributor Author

mmatera commented Jan 18, 2024

An canned image would also work.

In WMA, the errors are shown as "tooltips" (these yellow rectangles with a text that appears when the pointer passes over the image). We do not have that, but it would be nice to implement it. @TiagoCavalcante, do you think it would be possible, at least at the level of the graphics?

On the other hand, if the picture is exported as an image, the unique clue about one error happens is the pink background. Still, I could put the errors as an evaluation message.

@mmatera
Copy link
Contributor Author

mmatera commented Jan 19, 2024

I think now the behaviour is closer, but requires an slight change in Mathics-Django: Mathics3/mathics-django#198

Also, this makes that the option Background works again in Graphics and Graphics3D

@@ -1087,6 +1094,75 @@ def stylebox_style(style, specs):
raise BoxExpressionError
return new_style

failed = []

def build_error_box1(style):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we agree that setting the background is the right thing, these two functions can be safely removed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that setting the background is the right thing.

@rocky
Copy link
Member

rocky commented Jan 19, 2024

I think now the behaviour is closer, but requires an slight change in Mathics-Django: Mathics3/mathics-django#198

Also, this makes that the option Background works again in Graphics and Graphics3D

This is awesome. Thanks!

face=ERROR_BOX_FACE_COLOR,
)
if isinstance(self, GraphicsElements):
error_primitive_head = Symbol("PolygonBox")
Copy link
Member

@rocky rocky Jan 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's define PolygonBox and Polygon3DBox, in mathics.core.systemsymbols and move the assignment of the boxes here and below underneath the definitions of ERROR_BOX*COLOR. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the PR in Mathics-django was merged, this is no longer needed.

)
else:
error_primitive_head = Symbol("Polygon3DBox")
error_primitive_expression = Expression(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

@rocky
Copy link
Member

rocky commented Jan 19, 2024

With some small changes noted I think this is ready. It is much improved from the original draft. Thanks.

After this PR we are in a position now to do the relatively simple task of addressing the other kinds of place where we raise a graphics box error of some sort.

@rocky
Copy link
Member

rocky commented Jan 19, 2024

A tangential thought for the future. I wonder what the speedup would be if we pickle all the Symbols defined in system.symbols and pickle load them instead of evaluating mathics.core.systemsymbols ?

@mmatera mmatera force-pushed the handle_boxerror_in_graphics branch from 32c9389 to eac1c85 Compare January 19, 2024 15:27
@mmatera mmatera marked this pull request as ready for review January 19, 2024 15:40
@rocky
Copy link
Member

rocky commented Jan 19, 2024

LGTM. Now that we handle RGB opacity in #968 do we want to add 0.25 opacity to the background to match what you reported before?

(Or was that change made some other way? The display I see shows s slightly darker red than in the image of the issue)

@rocky
Copy link
Member

rocky commented Jan 19, 2024

The "Background" graphics option should be documented, but we can do that in a separate PR.

@mmatera mmatera merged commit d4f8b6b into master Jan 19, 2024
11 checks passed
@mmatera mmatera deleted the handle_boxerror_in_graphics branch January 19, 2024 17:46
@mmatera
Copy link
Contributor Author

mmatera commented Jan 19, 2024

LGTM. Now that we handle RGB opacity in #968 do we want to add 0.25 opacity to the background to match what you reported before?

This and the documentation for the Background property are going to be included in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants