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 :mesh3d seriesstyle for PyPlot backend #3835

Merged
merged 7 commits into from
Sep 22, 2021

Conversation

lmh91
Copy link
Contributor

@lmh91 lmh91 commented Sep 21, 2021

I added the support for the seriestype :mesh3d for the PyPlot backend.

The inputs are the vertices of a mesh and the connections have to be specified via the connections keyword
similar to https://docs.juliaplots.org/latest/generated/plotlyjs/#plotlyjs-ref47.
However, there is a difference in the syntax of how the connections are specified.
Here, it takes ::Vector{Vector{Ints}} where each element of the outer vector defines a single polygon.
Combinations of different polygons are possible as it is shown in the following example:

    using Plots
    pyplot()
    T = Float64
    p0 = T[0,0,0]
    p1 = T[1,0,0]
    p2 = T[0,1,0]
    p3 = T[1,1,0]
    p4 = T[1/2,1/2,1]
    pts = [p0, p1, p2, p3, p4]
    x, y, z = broadcast(i -> getindex.(pts, i), (1,2,3))

    plot(x, y, z, # [x[i],y[i],z[i]] is the i-th vertix of the mesh
        st=:mesh3d, 
        connections = [
            [1, 2, 4, 3], # Quadrangle 
            [1, 2, 5], # Triangle
            [2, 4, 5], # Triangle
            [4, 3, 5], # Triangle
            [3, 1, 5]  # Triangle
        ], 
        linecolor = :black,
        fillcolor = :blue,
        linewidth = 2,
        legend = true,
        label = "Pyramide",
        fillalpha = 0.2
    )

pyramide

I hope I did everything correct and please let me know if something should be implemented in a different way.

@t-bltg
Copy link
Member

t-bltg commented Sep 21, 2021

I'm wondering if we can unify all backends on the connections keyword, maybe not draw only triangles as with

function mesh3d_triangles(x, y, z, cns)
, but arbitrary polygons.

BTW, could you add the connections kw to the backends supporting mesh3d (GR, Gaston, others ?) in src/backends.jl: it seems they are missing and they now pop up as a warning because of #3816.

Please also update the Pyplot examples by removing #47 in the list of skipped examples:

:pyplot => [2, 25, 30, 31, 47, 49, 55],

for the docs.

Otherwise LGTM at first glance.

@t-bltg t-bltg added the extension new behaviour label Sep 21, 2021
@codecov
Copy link

codecov bot commented Sep 21, 2021

Codecov Report

Merging #3835 (2b4e74c) into master (0cd8124) will decrease coverage by 0.25%.
The diff coverage is 9.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3835      +/-   ##
==========================================
- Coverage   63.50%   63.25%   -0.26%     
==========================================
  Files          28       28              
  Lines        7464     7489      +25     
==========================================
- Hits         4740     4737       -3     
- Misses       2724     2752      +28     
Impacted Files Coverage Δ
src/backends.jl 63.47% <ø> (ø)
src/backends/pgfplotsx.jl 61.17% <0.00%> (-0.38%) ⬇️
src/examples.jl 61.97% <ø> (ø)
src/utils.jl 52.85% <11.11%> (-1.93%) ⬇️
src/args.jl 73.46% <0.00%> (-0.58%) ⬇️
src/backends/unicodeplots.jl 75.00% <0.00%> (ø)
src/backends/gr.jl 89.42% <0.00%> (+0.08%) ⬆️
src/colorbars.jl 92.72% <0.00%> (+1.81%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cd8124...2b4e74c. Read the comment docs.

@lmh91
Copy link
Contributor Author

lmh91 commented Sep 21, 2021

Thanks for the feedback.

I removed #47 for pyplot from the list of skipping examples and added the :connections kw to the backends which have :mesh3d in their list of supported seriestypes.

Hmm, I could add a method for mesh3d_triangles which dispatch on cns. I should be able to add support of the current syntax for only triangles to the pyplot backend.

@lmh91
Copy link
Contributor Author

lmh91 commented Sep 21, 2021

Okay, I did not add a new method but added the support of the current (only-triangle) syntax for :connections
directly in src/backends/pyplot.jl under st ==:mesh3d.

The example from the docs https://docs.juliaplots.org/latest/generated/plotlyjs/#plotlyjs-ref47
works now also with pyplot. I just added an alpha value here such that the structure becomes visible.

x = [0, 1, 2, 0]
y = [0, 0, 1, 2]
z = [0, 2, 0, 1]
i = [0, 0, 0, 1]
j = [1, 2, 3, 2]
k = [2, 3, 1, 3]

mesh3d(x, y, z; connections = (i, j, k), 
    title = "triangles", xlabel = "x", ylabel = "y", zlabel = "z", 
    legend = :none, margin = 2 * Plots.mm, alpha = 0.2)

example47

I am not fan of that syntax though as it uses 0-based indexing...

@t-bltg
Copy link
Member

t-bltg commented Sep 21, 2021

I am not fan of that syntax though as it uses 0-based indexing...

Yep, I don't know who chose this (one argument maybe is that most meshing libraries are written in C/C++/Python, hence the 0 based).

It bothers me a little to have 0 based indexing for triangles and 1 based for arbitrary polygons. What is the Matlab indexing based on for the triangulation in https://www.mathworks.com/help/matlab/ref/trimesh.html ? I think it's 1 based (https://mathworks.com/help/matlab/ref/delaunayn.html).

If it were me, I'd vote to unify this to 1 based indexing ...

@lmh91
Copy link
Contributor Author

lmh91 commented Sep 21, 2021

Sorry for the force push. I forgot to delete one debugging line in the first place.

@t-bltg
Copy link
Member

t-bltg commented Sep 21, 2021

Sorry for the force push. I forgot to delete one debugging line in the first place.

No worries, I also use (and certainly abuse) the force push 😆 .

@lmh91
Copy link
Contributor Author

lmh91 commented Sep 21, 2021

I would also vote for 1 and I would prefer Vector{NTuple{N, Int}} (length M) or Matrix{Int} (size = NxM)
so one would have a similar syntax for meshes only containing quadrangles.

As this would be a breaking change we could leave the 0-based syntax with a deprecation warning and add the new 1-based synatx in parallel. Since we could dispatch on NTuple{3, Vector{Int}} (current syntax) and Vector{NTuple{N,Int} or Matrix{Int} for meshes with only one type of polygon.

@t-bltg
Copy link
Member

t-bltg commented Sep 21, 2021

I would also vote for 1 and I would prefer Vector{NTuple{N, Int}} (length M) or Matrix{Int} (size = NxM)
so one would have a similar syntax for meshes only containing quadrangles.

As this would be a breaking change we could leave the 0-based syntax with a deprecation warning and add the new 1-based synatx in parallel. Since we could dispatch on NTuple{3, Vector{Int}} (current syntax) and Vector{NTuple{N,Int} or Matrix{Int} for meshes with only one type of polygon.

This looks reasonable, but it'd be better if we had complementary opinions on this matter.
Maybe @BeastyBlacksmith knows the reason behind 0 based indexing ?

@BeastyBlacksmith
Copy link
Member

mesh3d was started in #2909, so the answer to the indexing is probabably: because plotly does (pgfplots also IIRC)

@lmh91
Copy link
Contributor Author

lmh91 commented Sep 22, 2021

I would really suggest to use the 1-based julia way for the standard array types.

As Julia supports the definition of Array types which are 0-based (or actually any starting index),
one who would like to use 0-based indexing for the connections can do so, but would have to wrap his coordinate vectors x,
y and z into the 0-based array types. And internally the connections are anyhow increase with .+1
to setup the points for the plotly backend in mesh3d_triangles.

But I am open to implement it in any way. Just tell my :)

@BeastyBlacksmith
Copy link
Member

As this would be a breaking change we could leave the 0-based syntax with a deprecation warning and add the new 1-based synatx in parallel. Since we could dispatch on NTuple{3, Vector{Int}} (current syntax) and Vector{NTuple{N,Int} or Matrix{Int} for meshes with only one type of polygon.

I am okay with this. There is no need for users to wrap their coordinate vectors though, we can do the neccessary +/-1 internally.

@lmh91
Copy link
Contributor Author

lmh91 commented Sep 22, 2021

Okay, I added the 1-based syntax for :connections via dispatching on AbstractVector{NTuple{3,T}}.
The 0-based syntax still works with :connections <: Tuple{Array,Array,Array}.
I hope I did it for all backends which support :mesh3d correctly. I was not able to test all of them on my system.

I also updated the example #47.

  • GR
  • PyPlot
  • PlotlyJS
  • PGFPlotsX
  • Gaston

@t-bltg
Copy link
Member

t-bltg commented Sep 22, 2021

I was not able to test all of them on my system.

I've modified your post, could you tick which backends you have tested ? (GR is automagically tested in the CI).

@lmh91
Copy link
Contributor Author

lmh91 commented Sep 22, 2021

I changed the system and was able to test all (comparing the plots produced by the two different syntax) successfully.

@BeastyBlacksmith BeastyBlacksmith merged commit 866cb0c into JuliaPlots:master Sep 22, 2021
@BeastyBlacksmith
Copy link
Member

Thanks a lot. I would appreciate, if you can have a look into #3503 to see, if your information is correct.

@j-fu
Copy link

j-fu commented Sep 22, 2021

Hi, great work! I have seen this with the update to 1.22.2 . Is there a way to do Gouraud shading based on a vector of point colors or a colormap ? Can triangle colors be specified ? I am asking specifically for the GR backend, because it's fast, and OpenGL supports all this.

@t-bltg
Copy link
Member

t-bltg commented Sep 22, 2021

Hi, great work! I have seen this with the update to 1.22.2 . Is there a way to do Gouraud shading based on a vector of point colors or a colormap ? Can triangle colors be specified ? I am asking specifically for the GR backend, because it's fast, and OpenGL supports all this.

Not yet (it was evoked when I wrote #3612), but it shouldn't be too hard to implement if GR supports this ...

@lmh91
Copy link
Contributor Author

lmh91 commented Sep 22, 2021

I am actually in contact with Josef Heinen (GR.jl) regarding face colors. I hope we can add this soon.

@j-fu
Copy link

j-fu commented Sep 23, 2021

I am actually in contact with Josef Heinen (GR.jl) regarding face colors. I hope we can add this soon.

As I said, two modi would be interesting: node-wise coloring with color interpolation (Gouraud shading)
and face-wise coloring (flat shading). There is also Phong shading which interpolates normal. I find this less relevant as it would smooth out/hide discretization details, but that's a matter of taste.

With this functionality I could complete the Plots backend for GridVisualize.jl...

@lmh91
Copy link
Contributor Author

lmh91 commented Sep 23, 2021

I will communicate it to him.

BTW, for the PyPlot backend we could maybe also add some shading option which I found for Poly3DCollections:
https://stackoverflow.com/questions/56864378/how-to-light-and-shade-a-poly3dcollection
via mesh.set_facecolor(rgbNew) where rgbNew can be an vector of colors for each face.

@j-fu
Copy link

j-fu commented Sep 23, 2021

I will communicate it to him.

BTW, for the PyPlot backend we could maybe also add some shading option which I found for Poly3DCollections:
https://stackoverflow.com/questions/56864378/how-to-light-and-shade-a-poly3dcollection
via mesh.set_facecolor(rgbNew) where rgbNew can be an vector of colors for each face.

See also here: https://github.com/j-fu/GridVisualize.jl/blob/main/src/pyplot.jl#L497
So this is flat shading. I did not find Gouraud shading for this case.

OTOH pyplot is a bit overloaded performance-wise for this purpose, and the visual quality of the plots not that good (of course I could have missed some tricks here..).

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

Successfully merging this pull request may close these issues.

None yet

4 participants