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

IO: write_PLY() for Epeck #7874

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Conversation

afabri
Copy link
Member

@afabri afabri commented Nov 20, 2023

Summary of Changes

As reported in #7868 the function CGAL::IO::write_PLY() used in binary mode does not correctly write the coordinates if the points are from a kernel with exact constructions.
This PR applies `to_double()" to the coordinates.

After a discussion with @MaelRL we decided that the user is in charge to pass a vertex_point_map as named parameter that does the conversion. This is straightforward as we offer the Cartesian_converter_property_map.

As the problem is the same for the vertex normals we add a named parameter vertex_normal_map.

Todo

  • Fix the generic function write_polygon_mesh(). Currently it is fixed for Surface_mesh

Release Management

@sloriot
Copy link
Member

sloriot commented Nov 22, 2023

BGL tests failing in CGAL-6.0-Ic-112

@sloriot sloriot added Tests failing Batch_1 First Batch of PRs under testing and removed Under Testing labels Nov 22, 2023
@sloriot sloriot added Under Testing and removed Tests failing Batch_1 First Batch of PRs under testing labels Nov 22, 2023
@sloriot
Copy link
Member

sloriot commented Nov 29, 2023

Successfully tested in CGAL-6.0-Ic-116

@lrineau
Copy link
Member

lrineau commented Nov 29, 2023

@afabri There is still an undone TODO in the top message of this other PR. Do you want this PR merged as it is?

@afabri
Copy link
Member Author

afabri commented Nov 29, 2023

@afabri There is still an undone TODO in the top message of this other PR. Do you want this PR merged as it is?

No, I still want to fix the generic code.

@sloriot
Copy link
Member

sloriot commented Dec 14, 2023

@afabri any chance you can fix that today (if inclusion in 5.6.1 is needed)?

@sloriot
Copy link
Member

sloriot commented Jan 24, 2024

@afabri, this PR was marked tested 2 month ago and is still waiting for a TODO. Maybe you want to open an issue instead and integrate the PR.

@lrineau lrineau modified the milestones: 5.6.1, 5.6.2 Feb 28, 2024
@lrineau
Copy link
Member

lrineau commented May 28, 2024

Ping @afabri

@afabri
Copy link
Member Author

afabri commented May 28, 2024

Ping @afabri

Will be treated somewhere in the future.

@lrineau lrineau modified the milestones: 5.6.2, 6.1-beta May 30, 2024
@github-actions github-actions bot removed the Tested label Jun 7, 2024
Copy link

github-actions bot commented Jun 7, 2024

This pull-request was previously marked with the label Tested, but has been modified with new commits. That label has been removed.

@MaelRL MaelRL removed their request for review June 7, 2024 22:08
@MaelRL MaelRL self-assigned this Jun 7, 2024
@afabri afabri changed the base branch from 5.6.x-branch to master June 11, 2024 13:14
@afabri
Copy link
Member Author

afabri commented Jun 11, 2024

/build:v0

Copy link

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

@MaelRL MaelRL added the Not yet approved The feature or pull-request has not yet been approved. label Jun 18, 2024
@MaelRL
Copy link
Member

MaelRL commented Oct 22, 2024

Note that 6096748#diff-7a13764173912394a4970cfc328f818e714d19b9ac486d1ded974155934cdc8fR59 goes in the other direction from the current conclusion (users must give a VPM that converts to double), so this will need more discussions.

@lrineau
Copy link
Member

lrineau commented Oct 22, 2024

/build:v0

@lrineau lrineau self-requested a review October 22, 2024 16:08
Copy link

There was an error while building the doc:

/home/runner/work/cgal/cgal/Stream_support/include/CGAL/IO/PLY.h:356: warning: The following parameter of CGAL::IO::read_PLY(const std::string &fname, PointRange &points, PolygonRange &polygons, std::string &comments, const NamedParameters &np=parameters::default_values()) is not documented:
  parameter 'comments'
/home/runner/work/cgal/cgal/Stream_support/include/CGAL/IO/PLY.h:277: warning: The following parameter of CGAL::IO::read_PLY(std::istream &is, PointRange &points, PolygonRange &polygons, std::string &comments, const NamedParameters &np=parameters::default_values()) is not documented:
  parameter 'comments'
/home/runner/work/cgal/cgal/Surface_mesh/include/CGAL/Surface_mesh/IO/PLY.h:782: warning: argument 'verbose' of command @param is not found in the argument list of CGAL::IO::read_PLY(std::istream &is, Surface_mesh< P > &sm, std::string &comments, const NamedParameters &np=parameters::default_values())
/home/runner/work/cgal/cgal/Surface_mesh/include/CGAL/Surface_mesh/IO/PLY.h:782: warning: The following parameter of CGAL::IO::read_PLY(std::istream &is, Surface_mesh< P > &sm, std::string &comments, const NamedParameters &np=parameters::default_values()) is not documented:
  parameter 'np'

https://github.com/CGAL/cgal/actions/runs/11464164997

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not yet approved The feature or pull-request has not yet been approved. Pkg::Stream_support TODO
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Writing a binary PLY file does not work with an EPECK mesh
4 participants