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

Fix a DataInspector bug if inspector_label is used with RGB images #3468

Merged
merged 1 commit into from Jan 24, 2024

Conversation

Sagnac
Copy link
Contributor

@Sagnac Sagnac commented Dec 13, 2023

Description

If the creation of a custom tooltip was attempted with RGB type images the inspector would error after attempting to convert the Colorant to a Float32 using the Point3f constructor for the position parameter.

For cases in which RGB values are being plotted we now call the inspector_label function using a Tuple for its third argument instead, avoiding the error and allowing the RGB values to be used in the construction of the custom string.

Reproduction of the bug:

using GLMakie # git master
img = [RGBf(1.0, 1.0, 1.0) RGBf(1.0, 0.0, 0.0)
       RGBf(0.0, 1.0, 0.0) RGBf(0.0, 0.0, 1.0)]
figure, axis, plot = image(img; interpolate = false,
    inspector_label = (_, i, p) -> "$i = $(p[3])")
DataInspector(figure)
figure

which would produce

MethodError: Cannot `convert` an object of type ColorTypes.RGB{Float32} to an object of type Float32

As for the solution, we could skip the Colorant check and just return a Tuple instead, but if for whatever reason someone's script or package is currently expecting a Point3f type as before this would change that behaviour, so we call using a Tuple only when z is a Colorant; although using a Tuple universally here is much simpler of course.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

If the creation of a custom tooltip was attempted with RGB type images
the inspector would error after attempting to convert the Colorant to a
Float32 using the Point3f constructor for the position parameter.

For cases in which RGB values are being plotted we now call the
inspector_label function using a Tuple for its third argument instead,
avoiding the error and allowing the RGB values to be used in the
construction of the custom string.
@Sagnac
Copy link
Contributor Author

Sagnac commented Dec 13, 2023

Also I can switch the base branch to ff/DataInspector and update NEWS accordingly so that it's added with #3454 since that would probably be much more cohesive.

@ffreyer
Copy link
Collaborator

ffreyer commented Dec 13, 2023

Hmm, maybe it would be better to just return Point3f(x, y, 0) here and let users grab the color with the passed indices. The third component of a heatmap or image isn't actually a z coordinate after all, and the indices should be in a usable format.

@Sagnac
Copy link
Contributor Author

Sagnac commented Dec 13, 2023

i agree returning Point3f would be more consistent, but if interpolate = true the indices are interpolated floats so that would require an extra rounding step as well

@t-bltg t-bltg added the bug label Dec 23, 2023
Copy link
Collaborator

@ffreyer ffreyer left a comment

Choose a reason for hiding this comment

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

Let's merge this for now and leave the nitpicking about what exactly should be passed on for later. After all it also affects other plot types and it would probably be a more breaking change. (E.g. with extra output arguments or more separated arguments)

@SimonDanisch SimonDanisch merged commit 67c32ce into MakieOrg:master Jan 24, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants