Skip to content

Commit

Permalink
Replace Explicit_Cartesian_grid_geometry with Implicit_cartesian_grid…
Browse files Browse the repository at this point in the history 
…_geometry
  • Loading branch information
@JulyCode
JulyCode committed Jan 24, 2023
1 parent 98d0c11 commit bbd0255
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 50 deletions.
10 changes: 7 additions & 3 deletions Isosurfacing_3/include/CGAL/Isosurfacing_3/Explicit_Cartesian_grid_domain_3.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@

#include <CGAL/Isosurfacing_3/internal/Isosurfacing_domain_3.h>
#include <CGAL/Isosurfacing_3/internal/Explicit_Cartesian_grid_function.h>
#include <CGAL/Isosurfacing_3/internal/Explicit_Cartesian_grid_geometry_3.h>
#include <CGAL/Isosurfacing_3/internal/Implicit_Cartesian_grid_geometry_3.h>
#include <CGAL/Isosurfacing_3/internal/Grid_topology_3.h>
#include <CGAL/Isosurfacing_3/Zero_gradient.h>
#include <CGAL/Bbox_3.h>

namespace CGAL {
namespace Isosurfacing {
Expand All @@ -44,7 +45,7 @@ using Explicit_Cartesian_grid_domain_3 = unspecified_type;
template <typename Grid,
typename Gradient = Zero_gradient,
typename Topology = internal::Grid_topology_3,
typename Geometry = internal::Explicit_Cartesian_grid_geometry_3<Grid>,
typename Geometry = internal::Implicit_Cartesian_grid_geometry_3<typename Grid::Geom_traits>,

This comment has been minimized.

Sign in to view

Copy link
@MaelRL

MaelRL Jan 24, 2023

Member

The grid isn't necessarily a Cartesian_grid_3 object anymore. You could imagine passing a grid object whose point() is not a recomputation of the geometric position every time and thus the Explicit_Cartesian_grid_geometry_3 makes sense.
This also mixes explicit / implicit and thus makes the code more difficult to understand.

I would revert this commit.

This comment has been minimized.

Sign in to view

Copy link
@JulyCode

JulyCode Jan 25, 2023

Author Member

Hm, I see your point. I was going to argue that it would be better to separate the geometric information from the grid and instead use multiple grids for function values, gradient, and vertex positions. But I tried it in a new branch (you can have a look if you are interested) and came to the conclusion that it wouldn't improve as much as I thought. So maybe implementing different grid classes that either contain a gradient and/or explicit positions is the better idea. Right now the grid always uses memory for the gradient, even if it is not written to. Maybe that could be improved with this change as well.
So in conclusion, yes, I think you are right and you can revert the commit. Even though I don't like it that the grid now contains the same functionality as the Implicit_cartesian_grid_geometry.

This comment has been minimized.

Sign in to view

Copy link
@MaelRL

MaelRL Jan 25, 2023

Member

Even though I don't like it that the grid now contains the same functionality as the Implicit_cartesian_grid_geometry.

It looked somewhat weird that the class Cartesian_grid_3 doesn't have a geometry associated to it given that it was not a purely combinatorial representation of say a number of cells in each direction but it already had a geometric bounding box, and geometric spacing definition.

To hopefully clarify my changes, I see Cartesian_grid_3 as just a complete grid-with-value-and-gradient class, whose functors to interface it with the Isosurfacing_domain_3 (formely Base_domain) are trivial, but since `Explicit_..." is templated by the grid, you could now form an explicit grid domain with a grid class that would be far more incomplete (e.g. it could be a purely combinatorial grid for which a non-trivial, external "geometry functor" is required).

This comment has been minimized.

Sign in to view

Copy link
@JulyCode

JulyCode Jan 25, 2023

Author Member

Ok, I think I get what you mean. That would be an option to implement a new, more flexible grid class later on, should we feel like that is needed.
typename Function = internal::Explicit_Cartesian_grid_function<Grid> >
using Explicit_Cartesian_grid_domain_3 =
internal::Isosurfacing_domain_3<typename Grid::Geom_traits,
Expand Down Expand Up @@ -96,8 +97,11 @@ create_explicit_Cartesian_grid_domain(const Grid& grid,
const std::size_t size_j = grid.ydim();
const std::size_t size_k = grid.zdim();

const Bbox_3& bbox = grid.bbox();
const typename Geometry::Vector_3 offset{bbox.xmin(), bbox.ymin(), bbox.zmin()};

const Topology topo { size_i, size_j, size_k };
const Geometry geom { grid };
const Geometry geom { offset, grid.spacing() };
const Function func { grid };

return Domain{ topo, geom, func, grad, grid.geom_traits() };
Expand Down
47 changes: 0 additions & 47 deletions Isosurfacing_3/include/CGAL/Isosurfacing_3/internal/Explicit_Cartesian_grid_geometry_3.h
View file

This file was deleted.

0 comments on commit bbd0255

Please sign in to comment.