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

Add stop button for AW3 demo #7500

Merged
merged 3 commits into from
Jul 5, 2023
Merged

Add stop button for AW3 demo #7500

merged 3 commits into from
Jul 5, 2023

Conversation

MaelRL
Copy link
Member

@MaelRL MaelRL commented Jun 7, 2023

Release Management

  • Affected package(s): Polyhedron, Alpha_wrap_3
  • Issue(s) solved (if any): -
  • Feature/Small Feature (if any): n/a
  • License and copyright ownership: no change

MaelRL added 2 commits June 7, 2023 10:28
No changes to existing oracles as AW3's oracles use a shared ptr to AABB Tree
@MaelRL MaelRL added Enhancement Not yet approved The feature or pull-request has not yet been approved. CGAL 3D demo Pkg::Alpha_wrap_3 labels Jun 7, 2023
@MaelRL MaelRL added this to the 6.0-beta milestone Jun 7, 2023
@MaelRL MaelRL requested a review from lrineau June 7, 2023 08:36
Copy link
Member

@lrineau lrineau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have compile and tried the plugin, and reviewed as much code as possible. It seems fine. I have two minor comments.

Comment on lines +300 to +304
void emit_status()
{
Q_EMIT status_report(QString("%1 vertices").arg(wrapper.triangulation().number_of_vertices()));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is the only place where I might see an issue. But the Mesh_3 plugin has the same: the timer is created and started in the main thread, so this slot will be called by the main thread. The call wrapper.triangulation().number_of_vertices() is not thread-safe. However, the only consequence I can see is a possible incoherent number of vertices during one iteration of the slot.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is related to the macro CGAL_CONCURRENT_COMPACT_CONTAINER_APPROXIMATE_SIZE from PR #4389.

@sloriot sloriot added Batch_2 Second Batch of PRs under testing Under Testing and removed Under Testing labels Jun 8, 2023
@MaelRL MaelRL added the TODO label Jun 13, 2023
@sloriot
Copy link
Member

sloriot commented Jun 14, 2023

Successfully tested in CGAL-6.0-Ic-1

@sloriot sloriot removed the Batch_2 Second Batch of PRs under testing label Jun 15, 2023
@MaelRL MaelRL removed Not yet approved The feature or pull-request has not yet been approved. TODO Tested labels Jun 21, 2023
sloriot added a commit to sloriot/cgal that referenced this pull request Jun 22, 2023
@sloriot sloriot added Batch_1 First Batch of PRs under testing Under Testing and removed Under Testing Ready to be tested labels Jun 28, 2023
@sloriot sloriot added Tested and removed Batch_1 First Batch of PRs under testing labels Jul 5, 2023
@sloriot
Copy link
Member

sloriot commented Jul 5, 2023

Successfully tested in CGAL-6.0-Ic-15

@lrineau 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 Jul 5, 2023
@lrineau lrineau merged commit aca86b9 into CGAL:master Jul 5, 2023
@lrineau 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 Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants