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

Add more options to control the length of normals in MeshNormals visual #2043

Merged
merged 20 commits into from
Jun 1, 2021

Conversation

asnt
Copy link
Member

@asnt asnt commented May 30, 2021

Use cases:

  1. To get normals that are about the length across a triangle of the mesh:
MeshNormals(..., length_method='median_edge', length_scale=1.0)
MeshNormals(...)  # equivalent to the previous line (default values)
  1. To get normals that are about 10% the size of the mesh:
MeshNormals(..., length_method='max_extent', length_scale=0.1)

@djhoese djhoese changed the title Add more options to control the length of normals Add more options to control the length of normals in MeshNormals visual May 31, 2021
@djhoese
Copy link
Member

djhoese commented May 31, 2021

Would it be possible for you to add a test?

Also, what about some examples like the ones you listed in this PR's description but in an Examples section in the docstring. Like:

    **kwargs : dict, optional
                Extra arguments to define the appearance of lines. Refer to
                :class:`~vispy.visuals.line.LineVisual`.

    Examples
    -------------
    Normals at about the length of a triangle:

    ```
    MeshNormals(..., length_method='median_edge', length_scale=1.0)
    ```

    For normals at about 10% the size of the mesh:

And so on. Thoughts?

@asnt
Copy link
Member Author

asnt commented May 31, 2021

Would it be possible for you to add a test?

I will look at that next.

Also, what about some examples like the ones you listed in this PR's description but in an Examples section in the docstring.

Good idea. I had a try.

And so on. Thoughts?

I was thinking about making it more convenient to use this with a MeshVisual. Currently

mesh = MeshVisual(...)
normals = MeshNormals(mesh.mesh_data, ...)

Two ideas:

  1. normals = MeshNormals(visual=mesh)
  2. mesh = MeshVisual(..., normals='vertex')

With option 1, it is obvious where to configure the normals, at construction time or through normals.
With option 2, it is not obvious where to go to configure the normals and we'd need to expose mesh.normals, for example.

@asnt
Copy link
Member Author

asnt commented Jun 1, 2021

@djhoese In the tests, I set up a camera instead of defining the scene transformations manually. It seems simpler to maintain in my opinion. Is there maybe another reason for the manual transformations instead?

@asnt
Copy link
Member Author

asnt commented Jun 1, 2021

The error is when initializing the vispy-tests environment with micromamba:

/usr/bin/bash -c source /home/runner/.bash_profile && micromamba list
ERROR   Cannot activate, prefix does not exist at: /home/runner/micromamba/envs/vispy-tests
Error: The process '/usr/bin/bash' failed with exit code 1

I don't see at first how this could be linked to the last commit.

@kmuehlbauer
Copy link
Contributor

I don't see at first how this could be linked to the last commit.

@asnt Looks like a spurious thing. Maybe just rerun it will help?

@asnt
Copy link
Member Author

asnt commented Jun 1, 2021

@asnt Looks like a spurious thing. Maybe just rerun it will help?

Thanks @kmuehlbauer . I don't think I can relaunch the tests. Let's see with the new commit.

@asnt asnt force-pushed the mesh_normals-length branch from eea37e7 to 34acb77 Compare June 1, 2021 13:19
@asnt
Copy link
Member Author

asnt commented Jun 1, 2021

@djhoese The tests now cover all parameters. For length_method, the check is for it not to raise. Not sure how better to test it.
Any further suggestion?

@djhoese djhoese self-assigned this Jun 1, 2021
Copy link
Member

@djhoese djhoese left a comment

Choose a reason for hiding this comment

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

This looks really good. Nice job on the tests. One request: could you have the first Example in your docstring show you creating the Mesh visual and passing the meshdata instance to the normals visual?

@djhoese djhoese merged commit 22b9355 into vispy:main Jun 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants