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

GR: add support for mesh3d #3612

Merged
merged 2 commits into from
Jul 9, 2021
Merged

GR: add support for mesh3d #3612

merged 2 commits into from
Jul 9, 2021

Conversation

t-bltg
Copy link
Member

@t-bltg t-bltg commented Jul 4, 2021

Add GR support for mesh3d since, for now, GR does not support drawing arbitrary points using a triangle connectivity array.
The error check code was copied from the pgfplotsx backend: https://github.com/JuliaPlots/Plots.jl/blob/master/src/backends/pgfplotsx.jl

The test for this case already exists here as example 47, and is run e.g. for pgfplotsx: https://docs.juliaplots.org/latest/generated/pgfplotsx/#pgfplotsx-ref47.

Needs JuliaPlots/RecipesPipeline.jl#90 patch.

Output of example 47

ref47

Linked #392

@BeastyBlacksmith
Copy link
Member

This is missing the faces of the triangles though. Also some legend figure would be good.

@t-bltg
Copy link
Member Author

t-bltg commented Jul 5, 2021

This is missing the faces of the triangles though.

All right. But I would like to be able do disable faces via an option ...

Also some legend figure would be good.

OK I will change the example then.

@BeastyBlacksmith
Copy link
Member

You could also add a warning, that the GR backend doesn't support 3D mesh's with faces at the moment

@t-bltg t-bltg added the GR label Jul 5, 2021
@t-bltg t-bltg requested a review from daschw July 5, 2021 21:13
@t-bltg t-bltg added the enhancement improving existing functionality label Jul 6, 2021
@t-bltg t-bltg closed this Jul 6, 2021
@t-bltg t-bltg reopened this Jul 6, 2021
@t-bltg t-bltg closed this Jul 6, 2021
@t-bltg t-bltg reopened this Jul 6, 2021
@t-bltg t-bltg force-pushed the mesh3d branch 2 times, most recently from 5f195ae to 8ef52e0 Compare July 6, 2021 19:54
@codecov
Copy link

codecov bot commented Jul 6, 2021

Codecov Report

Merging #3612 (ed8fb4d) into master (ef93aa8) will decrease coverage by 1.53%.
The diff coverage is 35.23%.

❗ Current head ed8fb4d differs from pull request most recent head f5e6ae9. Consider uploading reports for the commit f5e6ae9 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3612      +/-   ##
==========================================
- Coverage   65.19%   63.66%   -1.54%     
==========================================
  Files          27       28       +1     
  Lines        6663     6885     +222     
==========================================
+ Hits         4344     4383      +39     
- Misses       2319     2502     +183     
Impacted Files Coverage Δ
src/backends.jl 61.06% <0.00%> (-2.25%) ⬇️
src/backends/gaston.jl 0.00% <0.00%> (ø)
src/examples.jl 61.76% <ø> (ø)
src/utils.jl 52.53% <54.54%> (-0.05%) ⬇️
src/backends/gr.jl 89.05% <88.09%> (+0.04%) ⬆️
src/axes.jl 84.75% <100.00%> (+0.72%) ⬆️
src/backends/pgfplotsx.jl 60.94% <100.00%> (ø)
src/init.jl 100.00% <100.00%> (ø)
src/layouts.jl 67.96% <100.00%> (-0.60%) ⬇️
src/pipeline.jl 89.63% <100.00%> (+0.51%) ⬆️
... and 6 more

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 ef93aa8...f5e6ae9. Read the comment docs.

@t-bltg t-bltg requested a review from BeastyBlacksmith July 6, 2021 20:13
@briederer
Copy link
Contributor

Quick question.
Could this mesh3d function be used to get a nicely working histogram3d (#3379)?
I started a PR (#3507) a few months ago but currently don't have any time to work on that 😕

So maybe my initial work on that could be finished with that PR being merged.

@t-bltg
Copy link
Member Author

t-bltg commented Jul 6, 2021

Quick question.
Could this mesh3d function be used to get a nicely working histogram3d (#3379)?
I started a PR (#3507) a few months ago but currently don't have any time to work on that confused

So maybe my initial work on that could be finished with that PR being merged.

I don't think so, since this is only for triangles, your vertical bars would be ugly ! Also, this is only for the GR backend.

However you mentioned in #3507 (comment) that surface didn't have a mesh option.

I've merged this PR which allows to use GR options when using surface, see the examples at #3591

Thinking out loud, maybe you can use:

surface(...)
surface!(..., display_option=GR.OPTION_SHADED_MESH)  # for a mesh on top of the previous plot ?

Is that of any help ?

@briederer
Copy link
Contributor

Thanks for the suggestions.
I somehow completely missed that this PR is only for triangles. 😅

I'll think about your suggestions, and if you happen to have another good idea comment on my PR #3507.

@t-bltg t-bltg force-pushed the mesh3d branch 3 times, most recently from 769bb19 to 49e87aa Compare July 8, 2021 09:24
@t-bltg t-bltg reopened this Jul 8, 2021
@t-bltg t-bltg marked this pull request as draft July 8, 2021 10:03
@t-bltg t-bltg marked this pull request as ready for review July 8, 2021 10:03
@t-bltg
Copy link
Member Author

t-bltg commented Jul 9, 2021

@BeastyBlacksmith, if you don't see anything against, can I merge this ?

@t-bltg t-bltg merged commit 5d608d1 into JuliaPlots:master Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improving existing functionality GR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants