issue #7395 Improvement of layout of model relations #7576

albert-github · 2023-07-05T10:14:44Z

Consistent implementation for the AABB_tree in respect to Has Model and Is Model Of

Consistent implementation for the `AABB_tree` in respect to `Has Model` and `Is Model Of`

sloriot · 2023-07-05T11:16:58Z

/build:v0

github-actions · 2023-07-05T11:20:29Z

The documentation is built. It will be available, after a few minutes, here: https://cgal.github.io/7576/v0/Manual/index.html

sloriot · 2023-07-05T11:39:10Z

Looks good to me!

sloriot · 2023-07-05T11:40:36Z

Maybe to be consistent with See also, it should be Has models since we cannot change See also.

albert-github · 2023-07-05T11:45:57Z

#7576 (comment)
I agree, but I just copied the term as it was, It also accounts for "Is Model Of" this probably should go the "Is model of"

There are probably a few, unrelated, others as well like:

Windows Demo
Common Demo
Optional Named Parameters
Classified Reference Pages

- Completed the cgalHasModel part - corrected spelling of `Has Model` and `Is Model Of` to `Has model` and `Is model of`

- Completed the cgalModels part

Based on a remark in CGAL#7576 (comment) aslo changed here some names for consistency

# Conflicts: # Polyhedron/doc/Polyhedron/Concepts/PolyhedronItems_3.h

albert-github · 2023-07-13T09:23:01Z

@sloriot @lrineau

Due to the new pull requests that were integrated into master this PR had a small problem building the documentation as it had merge conflicts (see https://cgal.geometryfactory.com/CGAL/Manual_doxygen_test/ and in particular https://cgal.geometryfactory.com/CGAL/Manual_doxygen_test/CGAL-6.0-Ic-22/logs1/Polyhedron.log).
This is not problem, though this merge conflict is not always noted, directly, by the author of the PR.

In the build sequence I see there is a "CMake Test Merge Branch / build (pull_request)" and I think during the overnight documentation build this is also run. When there is a merge conflict would it be possible to place a new comment with the PR as the submitter of the PR is normally notified of new comments and can take actions.

sloriot · 2023-07-13T15:54:25Z

Polyhedron/doc/Polyhedron/Concepts/PolyhedronItems_3.h

-\cgalHasModel `CGAL::Polyhedron_items_3`
-\cgalHasModel `CGAL::Polyhedron_min_items_3`
-\cgalHasModel `CGAL::Polyhedron_items_with_id_3`
+\cgalHasModelsBegin CGAL::Polyhedron_items_3


Suggested change

\cgalHasModelsBegin CGAL::Polyhedron_items_3

\cgalHasModelsBegin

\cgalHasModels CGAL::Polyhedron_items_3

(note that there are many such occurrences.)

that's what I noticed but then I don't understand these warnings: https://cgal.geometryfactory.com/CGAL/Manual_doxygen_test/CGAL-6.0-Ic-22/logs1/Polyhedron.log

The problem noted in the overnight build was due to the fact that there was a merge conflict.
(Note the difference cgalHasModel versus cgalHasModels (so with s).
I think the overnight build error should be gone due to the merge I did.

That was my first idea as well, but I had problems here regarding an extra empty line in the output due to the fact that we get an extra field (and thus extra unwanted empty space):

Here you see my implementation and the suggested change.

With the knowledge I now have, I think it would be possible to define:

"cgalHasModelsBegin=<dl><dt>@cgalHasModelsHeader</dt><dd>" \ "cgalHasModels{1}=<dt></dt><dd>`\1`</dd>" \ "cgalHasModels{2}=<dt></dt><dd>`\1,\2`</dd>" \ "cgalHasModelsEnd=</dl>" \

in case of

CGAL::Surface_mesh_shortest_path_traits<K,P>

this would also give the good results.

There are also some long model names like:

CGAL::Nonnegative_linear_program_from_iterators<A_it, B_it, R_it, FL_it, L_it, FU_it, U_it, D_it, C_it>

but this would use the cgalHasModels{1} as fallback.
Actually I'm not a fan of this fallback as it makes things less clear.

The initial idea I had was that the cgalHasModels would contain all the models for the list (like with cgalModels) and than the fallback would not work anymore.

In the mean time I also created a proposed pull request in doxygen (doxygen/doxygen#10179) that makes it possible to define a separator, so at that moment the fallback would not even be necessary anymore when we use as separator e.g. ;.

Conclusion I should probably rework the cgalHasModels part, using the {1} construct and the "fallback".
(I, probably, will have a look at it during the weekend).

- based on review

Correcting permissions

Corrected `cgalModels` to `cgalHasModes` inside `cgalHasNodelsBegin` / `cgalHasModelsEnd`

sloriot · 2023-07-19T13:40:44Z

I guess there is an unintended change:

https://cgal.geometryfactory.com/CGAL/Manual_doxygen_test/CGAL-6.0-Ic-24/output2/AABB_tree/classAABBPrimitive.html

albert-github · 2023-07-19T13:48:03Z

@sloriot that was indeed an unintended change but has been corrected yesterday as can be seen in https://cgal.geometryfactory.com/CGAL/Manual_doxygen_test/CGAL-6.0-Ic-25/output2/AABB_tree/classAABBPrimitive.html

albert-github · 2023-07-20T16:07:45Z

@sloriot as the item is still marked with "CI checks failed" and this was, as far as I know, based on CGAL-6.0-Ic-24 but this was already corrected for CGAL-6.0-Ic-25 (see also my comment #7576 (comment)) are there any other issues found so far?

sloriot

I wonder if for non-obvious construction we should not rely on the html code directly rather than tricking the macro.

Generalized_map/doc/Generalized_map/CGAL/Generalized_map.h

Generator/doc/Generator/CGAL/point_generators_2.h

sloriot · 2023-07-21T06:39:09Z

Kernel_23/doc/Kernel_23/CGAL/Projection_traits_xy_3.h

@@ -21,18 +21,12 @@ constructions or if `K` is a `CGAL::Filtered_kernel` (such as for
 `CGAL::Exact_predicates_inexact_constructions_kernel`), this class automatically
 provides exact predicates.

-\cgalModels The class is a model of several 2D triangulation traits class concepts,
+\cgalModelsBare{The class is a model of several 2D triangulation traits class concepts\,


Is there any reason why you choose different solutions for Models and HasModels?

I understand that you usually model only one concept but that a concept have several models.

The situation wit the cgalModels is that here the models don't have template like constrict when referring to it , whilst cgalHasModels does have it. For this with cgalHasModels the choice has been made to use a begin / end construct and with cgalModels a construct with the multi arguments was made. A few cases there were some "bare" constructs that needed some tricks, sometimes there was a , in the field and this could be prevented by means of the \, construct on other places multiple lines were necessary, hence the \n. This all could be overcome when the ALIASES had had the possibility to specify a separator, for this I did recently create the proposed pull request doxygen/doxygen#10179
Maybe we normally should use the cgalModels construct but in the case off cgalModelsBare use a begin / end construct as well to make it all more transparent.

sloriot · 2023-07-21T06:46:42Z

Number_types/doc/Number_types/CGAL/Sqrt_extension.h

-\cgalModels `ImplicitInteroperable` with `NT`
-\cgalModels `Fraction` if NT is a `Fraction`
-\cgalModels `RootOf_2`
+\cgalModelsBare{`Assignable`\n


This is strange construction IMO

See comment #7576 (comment)

Number_types/doc/Number_types/CGAL/long_long.h

sloriot · 2023-07-21T07:02:11Z

STL_Extension/doc/STL_Extension/Concepts/Hashable.h

-They can be disables by defining the macro `CGAL_DISABLE_HASH_OPENMESH`.
+\cgalHasModelsBegin
+\cgalHasModelsBare{All handles and indices of \cgal data structures}
+\cgalHasModelsBare{All handles of OpenMesh\, by including the specializations of the `graph_traits` header files provided by \cgal. They can be disables by defining the macro `CGAL_DISABLE_HASH_OPENMESH`.}


another construction not obvious for the dev (meaning we should add it to the dev doc at least)

See comment #7576 (comment)

sloriot · 2023-07-21T07:04:03Z

Set_movable_separability_2/doc/Set_movable_separability_2/Concepts/CastingTraits_2.h

@@ -7,7 +7,9 @@

  \cgalRefines{DefaultConstructible,PolygonTraits_2}

-  \cgalHasModel Any CGAL kernel, e.g., CGAL::Exact_predicates_exact_constructions_kernel.
+  \cgalHasModelsBegin
+  \cgalHasModelsBare{Any CGAL kernel, e.g., CGAL::Exact_predicates_exact_constructions_kernel.}


Suggested change

\cgalHasModelsBare{Any CGAL kernel, e.g., CGAL::Exact_predicates_exact_constructions_kernel.}

\cgalHasModelsBare{Any \cgal kernel, e.g., CGAL::Exact_predicates_exact_constructions_kernel.}

I'll correct this on.

I did a quick and dirty grep for CGAL in combination with cgal and got:

BGL/doc/BGL/PackageDescription.txt:584:\cgalPkgDescriptionBegin{CGAL and the Boost Graph Library,PkgBGL} CGAL_ipelets/doc/CGAL_ipelets/PackageDescription.txt:4:\cgalPkgDescriptionBegin{CGAL Ipelets,PkgCGALIpelets} GraphicsView/doc/GraphicsView/PackageDescription.txt:12:\cgalPkgDescriptionBegin{CGAL and the Qt Graphics View Framework,PkgGraphicsView} Point_set_processing_3/include/CGAL/pointmatcher/compute_registration_transformation.h:507: \cgalParamExtra{The logs generated by CGAL library does not get effected by this configuration.} STL_Extension/doc/STL_Extension/PackageDescription.txt:15:\cgalPkgDescriptionBegin{STL Extensions for CGAL,PkgSTLExtension} Shape_regularization/include/CGAL/Shape_regularization/regularize_contours.h:104: \cgalParamDefault{a CGAL `Kernel` deduced from the point type, Shape_regularization/include/CGAL/Shape_regularization/regularize_contours.h:227: \cgalParamDefault{a CGAL `Kernel` deduced from the point type, Shape_regularization/include/CGAL/Shape_regularization/regularize_segments.h:117: \cgalParamDefault{a CGAL `Kernel` deduced from the point type, Shape_regularization/include/CGAL/Shape_regularization/regularize_segments.h:355: \cgalParamDefault{a CGAL `Kernel` deduced from the point type, Shape_regularization/include/CGAL/Shape_regularization/regularize_segments.h:448: \cgalParamDefault{a CGAL `Kernel` deduced from the point type, Shape_regularization/include/CGAL/Shape_regularization/regularize_segments.h:541: \cgalParamDefault{a CGAL `Kernel` deduced from the point type, Shape_regularization/include/CGAL/Shape_regularization/regularize_segments.h:633: \cgalParamDefault{a CGAL `Kernel` deduced from the point type, Solver_interface/doc/Solver_interface/PackageDescription.txt:25:\cgalPkgDescriptionBegin{CGAL and Solvers, PkgSolverInterface}

maybe these should be looked at as well.

Spatial_searching/doc/Spatial_searching/CGAL/Search_traits_adapter.h

sloriot · 2023-07-21T07:16:32Z

Triangulation_2/doc/Triangulation_2/CGAL/Triangulation_face_base_with_info_2.h

-\cgalModels Because `Triangulation_face_base_with_info_2` derives from the class instantiating its third
-parameter, it will be a model of the same face base concept as its parameter:
-`TriangulationFaceBase_2`,  `ConstrainedTriangulationFaceBase_2`, or `RegularTriangulationFaceBase_2`
+\cgalModelsBare{Because `Triangulation_face_base_with_info_2` derives from the class instantiating its third


another non obvious

sloriot · 2023-07-21T07:16:52Z

Triangulation_2/doc/Triangulation_2/CGAL/Triangulation_vertex_base_with_info_2.h

@@ -22,10 +22,10 @@ the triangulation.
 this parameter is instantiated by
 `Triangulation_vertex_base_2<Traits>`.

-\cgalModels `TriangulationVertexBaseWithInfo_2`
-\cgalModels The parameter `Vb` is a model of some vertex base concept.
+\cgalModelsBare{`TriangulationVertexBaseWithInfo_2`,


another non obvious

Corrections after review.

albert-github · 2023-07-21T09:46:13Z

I corrected the obvious problems.

I didn't yet correct / update the cgalModelsBare problems as I'd like to see what the reaction is of the proposal in #7576 (comment) (i.e. begin / end construct).

I don't think that we should use "I wonder if for non-obvious construction we should not rely on the html code directly rather than tricking the macro." ( as suggested in #7576 (review)) as this might lead to code that is more difficult to understand, is more error prone and might lead to inconsistencies.

Replacing the `cgalModelsBare` construct with a `begin` / `end` construct so no less understandable constructs with `\,` and `\n` are necessary.

albert-github · 2023-07-21T17:29:50Z

Implemented also the begin / end for cgalModelsBare

lrineau · 2023-07-24T15:34:12Z

/build:v1

github-actions · 2023-07-24T15:38:27Z

The documentation is built. It will be available, after a few minutes, here: https://cgal.github.io/7576/v1/Manual/index.html

# Conflicts: # Arrangement_on_surface_2/doc/Arrangement_on_surface_2/Concepts/ArrangementPointLocation_2.h # Arrangement_on_surface_2/doc/Arrangement_on_surface_2/Concepts/ArrangementVerticalRayShoot_2.h

sloriot · 2023-09-06T09:50:05Z

Successfully tested in CGAL-6.0-Ic-56

sloriot · 2023-09-06T09:50:51Z

Output looks good.
now:

before:

sloriot · 2023-09-06T09:51:43Z

Thanks @albert-github for the huge work!

issue CGAL#7395 Improvement of layout of model relations

c0db92d

Consistent implementation for the `AABB_tree` in respect to `Has Model` and `Is Model Of`

albert-github mentioned this pull request Jul 5, 2023

Improvement of layout of model relations. #7395

Closed

sloriot added Enhancement Doc labels Jul 5, 2023

lrineau linked an issue Jul 5, 2023 that may be closed by this pull request

Improvement of layout of model relations. #7395

Closed

albert-github added 2 commits July 6, 2023 17:35

issue CGAL#7395 Improvement of layout of model relations

ee2a552

- Completed the cgalHasModel part - corrected spelling of `Has Model` and `Is Model Of` to `Has model` and `Is model of`

issue CGAL#7395 Improvement of layout of model relations

301728a

- Completed the cgalModels part

albert-github added a commit to albert-github/cgal that referenced this pull request Jul 9, 2023

Consistency of terms

77020e9

Based on a remark in CGAL#7576 (comment) aslo changed here some names for consistency

albert-github mentioned this pull request Jul 9, 2023

Consistency of terms #7591

Merged

sloriot added the Under Testing label Jul 11, 2023

Merge branch 'master' into feature/issue_7395

a0320dd

# Conflicts: # Polyhedron/doc/Polyhedron/Concepts/PolyhedronItems_3.h

sloriot reviewed Jul 13, 2023

View reviewed changes

MaelRL added the Work in progress label Jul 13, 2023

albert-github added 3 commits July 15, 2023 13:12

issue CGAL#7395 Improvement of layout of model relations

4e5578d

- based on review

issue CGAL#7395 Improvement of layout of model relations

894b2e2

Correcting permissions

Merge remote-tracking branch 'upstream/master' into feature/issue_7395

de3ba0f

albert-github marked this pull request as draft July 18, 2023 09:51

issue CGAL#7395 Improvement of layout of model relations

ad41766

Corrected `cgalModels` to `cgalHasModes` inside `cgalHasNodelsBegin` / `cgalHasModelsEnd`

albert-github marked this pull request as ready for review July 18, 2023 11:32

sloriot added CI checks failed and removed Under Testing labels Jul 19, 2023

sloriot removed CI checks failed Work in progress labels Jul 21, 2023

sloriot reviewed Jul 21, 2023

View reviewed changes

issue CGAL#7395 Improvement of layout of model relations

e4dff1f

Corrections after review.

This comment was marked as outdated.

Sign in to view

issue CGAL#7395 Improvement of layout of model relations

3205466

Replacing the `cgalModelsBare` construct with a `begin` / `end` construct so no less understandable constructs with `\,` and `\n` are necessary.

Merge remote-tracking branch 'upstream/master' into feature/issue_7395

ff3ff12

sloriot added the Under Testing label Aug 15, 2023

sloriot and others added 2 commits August 15, 2023 07:16

Merge remote-tracking branch 'cgal/master'

28f904f

Merge branch 'master' into feature/issue_7395

9300130

# Conflicts: # Arrangement_on_surface_2/doc/Arrangement_on_surface_2/Concepts/ArrangementPointLocation_2.h # Arrangement_on_surface_2/doc/Arrangement_on_surface_2/Concepts/ArrangementVerticalRayShoot_2.h

sloriot added Tested and removed Under Testing labels Sep 6, 2023

lrineau added this to the 6.0-beta milestone Sep 6, 2023

lrineau self-assigned this Sep 6, 2023

lrineau added the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Sep 6, 2023

lrineau merged commit 4692334 into CGAL:master Sep 7, 2023
9 checks passed

lrineau removed the rm only: ready for master For the release team only: that indicates that a PR is about to be merged in 'master' label Sep 7, 2023

lrineau deleted the feature/issue_7395 branch September 7, 2023 09:07

lrineau mentioned this pull request Oct 16, 2023

issue #7767 Problem with the documenation of Mesh_3 #7768

Merged

lrineau added the Merged_in_6.0 label Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue #7395 Improvement of layout of model relations #7576

issue #7395 Improvement of layout of model relations #7576

albert-github commented Jul 5, 2023

sloriot commented Jul 5, 2023

github-actions bot commented Jul 5, 2023

sloriot commented Jul 5, 2023

sloriot commented Jul 5, 2023

albert-github commented Jul 5, 2023

albert-github commented Jul 13, 2023

sloriot Jul 13, 2023

MaelRL Jul 13, 2023

sloriot Jul 13, 2023

albert-github Jul 13, 2023 •

edited

Loading

albert-github Jul 13, 2023

sloriot commented Jul 19, 2023 •

edited

Loading

albert-github commented Jul 19, 2023 •

edited

Loading

albert-github commented Jul 20, 2023

sloriot left a comment

sloriot Jul 21, 2023

sloriot Jul 21, 2023 •

edited

Loading

albert-github Jul 21, 2023

sloriot Jul 21, 2023

albert-github Jul 21, 2023

sloriot Jul 21, 2023

albert-github Jul 21, 2023

sloriot Jul 21, 2023

albert-github Jul 21, 2023 •

edited

Loading

sloriot Jul 21, 2023

sloriot Jul 21, 2023

albert-github commented Jul 21, 2023 •

edited

Loading

This comment was marked as outdated.

albert-github commented Jul 21, 2023

lrineau commented Jul 24, 2023

github-actions bot commented Jul 24, 2023

sloriot commented Sep 6, 2023

sloriot commented Sep 6, 2023

sloriot commented Sep 6, 2023

	\cgalHasModelsBegin CGAL::Polyhedron_items_3
	\cgalHasModelsBegin
	\cgalHasModels CGAL::Polyhedron_items_3

	\cgalHasModelsBare{Any CGAL kernel, e.g., CGAL::Exact_predicates_exact_constructions_kernel.}
	\cgalHasModelsBare{Any \cgal kernel, e.g., CGAL::Exact_predicates_exact_constructions_kernel.}

issue #7395 Improvement of layout of model relations #7576

issue #7395 Improvement of layout of model relations #7576

Conversation

albert-github commented Jul 5, 2023

sloriot commented Jul 5, 2023

github-actions bot commented Jul 5, 2023

sloriot commented Jul 5, 2023

sloriot commented Jul 5, 2023

albert-github commented Jul 5, 2023

albert-github commented Jul 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albert-github Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloriot commented Jul 19, 2023 • edited Loading

albert-github commented Jul 19, 2023 • edited Loading

albert-github commented Jul 20, 2023

sloriot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloriot Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albert-github Jul 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albert-github commented Jul 21, 2023 • edited Loading

This comment was marked as outdated.

albert-github commented Jul 21, 2023

lrineau commented Jul 24, 2023

github-actions bot commented Jul 24, 2023

sloriot commented Sep 6, 2023

sloriot commented Sep 6, 2023

sloriot commented Sep 6, 2023

albert-github Jul 13, 2023 •

edited

Loading

sloriot commented Jul 19, 2023 •

edited

Loading

albert-github commented Jul 19, 2023 •

edited

Loading

sloriot Jul 21, 2023 •

edited

Loading

albert-github Jul 21, 2023 •

edited

Loading

albert-github commented Jul 21, 2023 •

edited

Loading