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

fix refinement crash when face has more than 4 vertices #50556

Merged
merged 2 commits into from Oct 20, 2022

Conversation

vcloarec
Copy link
Member

fix #50541
The issue comes from that refinement of faces with vertices count greater than 4 is not allowed.

Also change the UI to not purpose to refine face if its vertices count is greater than 4.

Thanks @uclaros for your tries and to have report it!

@github-actions github-actions bot added this to the 3.28.0 milestone Oct 14, 2022
@vcloarec vcloarec added Crash/Data Corruption Mesh Related to general mesh layer handling (not specific data formats) backport release-3_22 Bug Either a bug report, or a bug fix. Let's hope for the latter! labels Oct 14, 2022
@uclaros
Copy link
Contributor

uclaros commented Oct 18, 2022

Wouldn't it be useful if refinement would still work on more than four vertices faces but with a different logic?
Once you delete a few edges and end up with a face with more than 4 vertices it is not very easy to re-create triangles: You need to completely remove the face, then select the vertices that surround the hole and do right-click -> Delaunay Triangulation

This could be handled in one step by refine face, when there are more than 4 vertices.

@vcloarec
Copy link
Member Author

good point @uclaros, again !
I understand the difficulties but with the logic of this refinement, it could quite complex to handle faces with more than 4 vertices.
What needed here could be to purpose another command to triangulate faces with more than 3 vertices, this will be a new feature.

@uclaros
Copy link
Contributor

uclaros commented Oct 18, 2022

What needed here could be to purpose another command to triangulate faces with more than 3 vertices, this will be a new feature.

This is what I meant, this new command could still be triggered by the same Refine Face action. If there are less than 4 vertices, use the old refinement logic. If there are more than 4, use the new triangulate logic.

@vcloarec
Copy link
Member Author

vcloarec commented Oct 18, 2022

this new command could still be triggered by the same Refine Face action

not sure, it will not exactly a refinement but a triangulation, not exactly the same. This refinement consists of adding new vertices in middle of edges. Triangulation, there i no more vertices, only a triangulation of a polygon. This is not exactly the same operation.

@uclaros
Copy link
Contributor

uclaros commented Oct 18, 2022

OK, still the two actions could occupy the same position in the context menu and only one of them visible at a time, depending on number of vertices.

@vcloarec
Copy link
Member Author

Yes, when this new action will be implemented, this action will replace the refinement action if the current face has more than 4 vertices.
If there are more than one faces selected, that will depends on the vertices count of each faces:

  • If no faces with more than 4 vertices: only refinement,
  • If no face with less than 5 vertices: only triangulation,
  • if both:refinement and triangulation.

But it is another story, and I think it could a be a new feature PR in the future.

Copy link
Contributor

@uclaros uclaros left a comment

Choose a reason for hiding this comment

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

Lgtm

@vcloarec vcloarec merged commit 350bcd4 into qgis:master Oct 20, 2022
@qgis-bot
Copy link
Collaborator

The backport to release-3_22 failed:

The process '/usr/bin/git' failed with exit code 1
stderr
error: could not apply f982262b83... fix refinment crash when face has more than 4 vertices
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

stdout
Auto-merging src/app/mesh/qgsmaptooleditmeshframe.cpp
CONFLICT (content): Merge conflict in src/app/mesh/qgsmaptooleditmeshframe.cpp
Auto-merging src/app/mesh/qgsmaptooleditmeshframe.h
Auto-merging src/core/mesh/qgsmeshadvancedediting.cpp
Auto-merging tests/src/core/testqgsmesheditor.cpp

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-3_22 release-3_22
# Navigate to the new working tree
cd .worktrees/backport-release-3_22
# Create a new branch
git switch --create backport-50556-to-release-3_22
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick f982262b8373bd71c061b044cff1ea49258a3bf5,67a365be13fd39cc21450117eb0799658034d754
# Push it to GitHub
git push --set-upstream origin backport-50556-to-release-3_22
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-3_22

Then, create a pull request where the base branch is release-3_22 and the compare/head branch is backport-50556-to-release-3_22.

@qgis-bot qgis-bot added the failed backport The automated backport attempt failed, needs a manual backport label Oct 20, 2022
vcloarec added a commit to vcloarec/QGIS that referenced this pull request Oct 20, 2022
marcu pushed a commit to marcu/QGIS that referenced this pull request Oct 23, 2022
* fix refinment crash when face has more than 4 vertices

* complete test
nyalldawson pushed a commit that referenced this pull request Nov 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter! Crash/Data Corruption failed backport The automated backport attempt failed, needs a manual backport Mesh Related to general mesh layer handling (not specific data formats)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when using Refine Current Face
3 participants