-
Notifications
You must be signed in to change notification settings - Fork 114
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
Adding 3D isosurface visualization #1076
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1076 +/- ##
===========================================
+ Coverage 83.01% 96.75% +13.75%
===========================================
Files 303 303
Lines 23906 23968 +62
===========================================
+ Hits 19844 23190 +3346
+ Misses 4062 778 -3284
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I should note that the isosurface looks significantly fancier even if it's just colored according to a random quantity, such as the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's great to see additional plotting functionality coming to Trixi! However, I have a few questions regarding the feasibility of this addition, and some minor comments. Please also note that this is not a full review, for which I currently do not have the resources.
@@ -10,6 +10,7 @@ EllipsisNotation = "1.0" | |||
FillArrays = "0.13.2" | |||
ForwardDiff = "0.10.18" | |||
GeometryBasics = "0.3, 0.4" | |||
GridVisualize = "0.5" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me like this feature adds a lot of plotting related dependencies (each of which have their own dependencies). I wonder if this is getting out of hand somewhat, but I don't have a good solution to it either. Maybe we should start thinking about a "TrixiVisualize.jl" or "TrixiPlots.jl" package?
cc @ranocha @jlchan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good idea. We should probably discuss this further also at one of the next meetings.
using Pkg; Pkg.activate(temp=true); | ||
Pkg.add("OrdinaryDiffEq"); | ||
Pkg.develop("Trixi"); | ||
Pkg.add("GLMakie") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think this belongs in the visualization docs.
@@ -1406,6 +1406,47 @@ function global_plotting_triangulation_makie(pds::PlotDataSeries{<:PlotData2DTri | |||
return plotting_mesh | |||
end | |||
|
|||
# 3D isosurface case for global_plotting_triangulation_makie function | |||
function global_plotting_triangulation_makie(plot_data::PlotData3DTriangulated{<:ScalarData}, level) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is not very descriptive to me - I think it would be good to elaborate at least a little
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm stretched a bit too thin this week - please work on the comments of @sloede and re-request our review (his and mine) again afterwards
Currently, Trixi does not support plotting 3D isosurfaces. These changes will support this functionality. Currently, the way to create a 3D isosurface is to create a
pd = ScalarPlotData3D(some_scalar_array, semi)
and then useiplot(pd)
. The idea is thatiplot
can have thei
stand for either "interactive" or "isosurface". We have also added documentation.Here is an example plot for the Taylor-Green vortex at time
t=2.5
.The one downside to this implementation (due to the packages we use for isosurface extraction) is that we cannot color the isosurface by another value yet. This should be added in a future release of GridVisualize.jl WIAS-PDELib/GridVisualize.jl#13.