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

in (/vispy/visuals /surface_plot.py) line 129 #2542

Open
Abbas-Yasir opened this issue Oct 24, 2023 · 2 comments
Open

in (/vispy/visuals /surface_plot.py) line 129 #2542

Abbas-Yasir opened this issue Oct 24, 2023 · 2 comments

Comments

@Abbas-Yasir
Copy link

    if colors.ndim == 3:
        # convert (width, height, 4) to (num_verts, 4)
        vert_shape = self.__vertices.shape
        num_vertices = vert_shape[0] * vert_shape[1]
        colors = colors.reshape(num_vertices, **_3_**)
    return colors

Shouldn't:
colors = colors.reshape(num_vertices, 3)
be
colors = colors.reshape(num_vertices, 4) ????

@djhoese
Copy link
Member

djhoese commented Oct 24, 2023

FYI you can link to exact lines and line ranges with a github URL. You can even use a "permalink" and github will show a preview of the code in your comment. Here is what you are referencing:

def _prepare_mesh_colors(self, colors):
if colors is None:
return
colors = np.asarray(colors)
if colors.ndim == 3:
# convert (width, height, 4) to (num_verts, 4)
vert_shape = self.__vertices.shape
num_vertices = vert_shape[0] * vert_shape[1]
colors = colors.reshape(num_vertices, 3)
return colors

Let me review the code and see what I think. I feel like this came up in another issue recently.

@djhoese
Copy link
Member

djhoese commented Oct 24, 2023

Overall I think you're right. However, two things:

  1. This should have raised an error as it is with every usage of RGBA colors being passed. So people must have gotten used to only passing RGB colors. If you used 4 in the reshape call and the input was RGB (3 element color) then it would fail.
  2. Instead of 4, it should just be -1 and numpy will put whatever remaining elements there are in that last dimension. Or it could be colors.shape[-1] which is maybe a bit less error prone. Actually, it could be .reshape((-1, colors.shape[-1])) and then that num_vertices call doesn't need to happen at all...but I suppose that validates that colors are the same size.

Anyway, the mesh converts RGB (3) to RGBA (4) here inside the mesh:

if colors.shape[-1] == 3:
pad = np.ones((len(colors), 1), colors.dtype)
if colors.ndim == 3:
pad = pad[:, :, np.newaxis]
colors = np.concatenate((colors, pad), axis=-1)

I'd appreciate a pull request with changing the 3 to a -1 if you're up to it?

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

No branches or pull requests

2 participants