Refactor MeshVisual indexing for easier and more flexible filter creation

[asnt]

MeshVisual currently seems to always reindex the vertex array as face-indexed vertices. This should rather be the decision of the caller for practical and perfomance reasons (e.g., glDrawEelements vs glDrawArrays).

This PR changes the behavior of MeshVisual to use whatever indexing was provided by the caller.

This allows the implementation of texture filters (#1444) and coming-up shading filters without workarounds.

[asnt]

The scene widget and a few visuals (box, cub, plane, polygon, sphere) rely on the current default face-indexed mode . Adding the indexing=None flag, as in 9040bae and ef061c8, is perhaps not the best solution for backwards compatibility but shows that something can be done about it.
Possible alternatives:

• Use indexing='faces' as default (does not seem natural)
• Detect the indexing mode automatically from the data. Not sure if at all possible. E.g., faces=None means face-indexed. There are other cases I don't undertand that arose from trying to implement that.

@campagnola @kmuehlbauer @larsoner @davidh-ssec Any feedback on this?

[djhoese]

@asnt I finally have a little time to look at all of our recent PRs (even if this is a month old, sorry). However, I'm not really familiar with the MeshVisual code. I do have some questions from what I can see.

1. What happened to all of the mesh logic for 'smooth' and getting face normals?
2. What about the visuals you mentioned requires the 'faces' indexing? I think it would make sense to have indexing=None as default and figure out a good default based on what parameters they specified. Additionally, it should error out if they provide parameters and request an indexing that doesn't make sense (not sure if that is possible).
3. indexing versus indexed seems a bit inconsistent.

[asnt]

Thank you for following up @djhoese.

Here are the answers to the questions based on what I can recall:

1. I found the logic in _update_data() in the mesh object to be broken (maybe I am wrong), so I tried to fix it. In the original code, there is an if/else branching. The first branch is taken for shading="smooth" when the MeshData object md doesn't have "face indexed" data. In that case, there is a call to get the normals md.get_vertex_normals(). Until here all seems fine. The problem is that when getting those normals, the state of the MeshData object systematically changes (at least if it has to internally compute the normals from the triangles) so that md.has_face_indexed_data() becomes True. This means that, on subsequent calls to _update_data(), the first path of the branch is never taken again. I don't think this is expected.
2. Yes, I thought about guessing the "indexing" mode from the parameters but could not come up with a working solution. I don't exclude the possibility that there is one still. What seems likely (with my modest understanding of computer graphics) is that when passing vertex_colors, the buffers have to contain the replicated vertices (i.e., what is called indexed='faces' in the old code). At least some of the visuals I touched at define vertex_colors. Depending on how flat shading is implemented in the shader, it might be necessary to use indexed='faces' also. (In PR Add ShadingFilter for meshes by separating it from MeshVisual #1463, there is a different implementation of flat shading that works independently of the indexing mode.)
3. Sure. I find indexed=None|'faces' confusing. In my understanding, indexed attemps to refer to whether the vertices array is indexed by the faces array. So, I think indexed (or indexing, whatever is more appropriate) should be a boolean. If indexed=True, the faces array indexes the vertices array. If indexed=False, there is no faces array. In that case, the faces are defined by the order of the vertex positions in the vertices array (e.g., contiguous triplets of vertex positions for mode='triangle'). So, the current indexed=None would really mean indexed=True in the proposed interpretation, and indexed='faces' would mean indexed=False.

I think we should profit from the occasion to extend MeshVisual with a Material API as discussed in campagnola#10 (comment)
Maybe we should take inspiration from Three.js

[larsoner]

Looks like this PR is a blocker for some potentially nice progress! I tried it out on an example I had, it didn't work, so I trimmed the example down:

import numpy as np
from vispy import scene

canvas = scene.SceneCanvas(keys='interactive', bgcolor='w')
grid = canvas.central_widget.add_grid(spacing=0, margin=0)
view = grid.add_view()
rr = np.array([[-1, -1, 0], [1, -1, 1.], [1., 1., 0.], [-1, 1, 0]])
tris = np.array([[0, 1, 2], [0, 2, 3]])
vertex_colors = [[1, 0, 0], [0, 1, 0], [0, 0, 1], [0, 0, 0]]
mesh = scene.visuals.Mesh(vertices=rr, faces=tris,
                          vertex_colors=vertex_colors, shading='smooth')
mesh.shininess = 0.1
mesh.ambient_light_color = (0.5, 0.5, 0.5)
mesh.light_dir = (-1, 1, -1)
view.add(mesh)
view.camera = scene.TurntableCamera(
    up='+z', elevation=70, azimuth=30, fov=0)
view.camera.scale_factor = 3
canvas.show()

On master I get what I expect, a "bent" square with a shiny corner:

On this PR it changes and looks incorrect:

Do you have time to look into it?

[asnt]

@larsoner After some thought, I propose another (simpler) strategy in this updated PR: Always send the element arrays (i.e. no index arrays) to the graphics card.
Rationale:

• Does not break the MeshVisual API
• Indexed arrays passed to the constructor can always be converted to full element arrays (as far as I can tell)
• Avoids the inconsistency in _update_data() mentioned above (Refactor MeshVisual indexing for easier and more flexible filter creation #1462 (comment)) allowing an easier implementation of TextureFilter, ShadingFilter...

The test above seems to give the expected result:

[larsoner]

As long as internally everything works and externally users can work in indexed mode like they could before I'm fine with it

[djhoese]

@asnt Do you foresee any performance issues by sending the full faces instead of index arrays? I'm guessing even if there are that the benefits of filters working/being easier make it worth it?

[djhoese]

I was going to comment and say that I'm worried this is doing a lot of extra work if someone provides their data in a draw-able way, but now realize that these computations were being done already (getting faces if we were only getting vertices, etc). I'm ok merging this if @asnt can comment/fix the things I pointed out about unnecessary code.

Any issues that this uncovers with users' use cases or if this limits functionality in any way can be dealt with in the future. The number of features that @asnt has been requesting/adding and that need this fix/clarification is more of a driving reason to merge this than keep the old functionality to support some obscure previous use case (not saying backwards compatibility isn't important). It becomes more of an argument to take this code that is cleaner and easier to understand and make it work with any broken use cases, if any, than to support the current implementation. All this is to say, I'm not 100% sure all this is perfect, but it doesn't seem less perfect than the code that is there already.

[sofroniewn]

I just tried this out, it works fine for me all my use cases - including after dropping the lines @djhoese flagged as not needed (which should be done before this gets merged).

I did notice a problem with flat and smooth shading when using a 2D mesh - which is the hardcoding of some some 3D assumptions here

vispy/vispy/geometry/meshdata.py

Line 339 in 9e0822d

self._vertex_normals[vindex] = (0, 0, 0)

and here

vispy/vispy/geometry/meshdata.py

Line 307 in 9e0822d

norms = np.empty((self._face_normals.shape[0], 3, 3),

but that is really independent of this PR and would have been broken before hand. When the time is right I can make a short follow-up PR to make sure the those lines do something reasonable when given 2D meshes if a shading mode is enabled

[asnt]

@asnt Do you foresee any performance issues by sending the full faces instead of index arrays? I'm guessing even if there are that the benefits of filters working/being easier make it worth it?

@djhoese I think the indexed vertices are advantageous for larger meshes or for collections of meshes sharing the same geometry. I might be missing some other aspects, though. However, sticking to faces internally makes indeed the implementation simpler while still general as far as I can tell.

[djhoese]

This might be a good pull request for @bobzwik to test. If you have the time, that would be great. If I understood what @bobzwik was doing in #1814, they may be taking advantage of the index array passing that could be effected by this PR (although probably not noticeably).

[djhoese]

@asnt There was a failing test. I assumed it was random and restarted it. We'll see how that goes.

Could you remind me what the situation with this PR is versus the other ones that you've created? #1444 is ready to merge and works as-is, but could benefit from this PR to simplify some of its logic, right? #1463 relies on this and won't be able to work without it, right?

So we could merge #1444 now, double check this and merge this PR, then update/remerge #1463 and merge that in to master, and then make another PR to simplify the TextureFilter to use the new indexing/features added in this PR. Does that sound good? Any other PR I'm forgetting about (looking through my open browser tabs now)?

[djhoese]

Ah there is also #1689 which says it depends on #1463. That could be updated after #1463 is merged and at the same time as TextureFilter from #1444 is re-factored. Again, let me know if any of this is out of order.

[asnt]

@djhoese The dependency order of the PR's you suggested seems right:

1. Refactor MeshVisual indexing for easier and more flexible filter creation #1462
2. Add ShadingFilter for meshes by separating it from MeshVisual #1463
3. Add mesh wireframe filter #1689
4. Update Add TextureFilter for adding textures to MeshVisuals #1444 to work with Refactor MeshVisual indexing for easier and more flexible filter creation #1462 (Already done in Add TextureFilter for adding textures to MeshVisuals #1444)

I've got merge conflicts with this branch at the moment. I'll have a closer look later if still open.

[djhoese]

I was going to ask everyone if it is OK to merge this but now realize (after looking at the "Files changed") that this includes the important changes from #1444. So 🤦‍♂️ on my part, but merging this now and will continue on in working on the related PRs.

[djhoese]

@asnt Given my last comment's realization (that #1444 includes the changes in this PR) do you think number 4 in your above list is something we still need to worry about?

[asnt]

@asnt Given my last comment's realization (that #1444 includes the changes in this PR) do you think number 4 in your above list is something we still need to worry about?

@djhoese Indeed, point 4 seems done already. I thought the indexing in the TextureFilter would need updating but, looking at it again, it seems I already did it (as mentioned in this edit).