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 method to inset polygon vertices #106

Closed
wants to merge 3 commits into from
Closed

Add method to inset polygon vertices #106

wants to merge 3 commits into from

Conversation

zilmarinen
Copy link
Contributor

Description

Implements a new method to inset the vertices of a polygon by a given amount by iterating through edge segment pairs to find a common point of intersection between the two translated planes.

polygon_inset

Scope of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • The project builds successfully without generating errors
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • No force unwrapping and all optionals are guarded against / safely checked

Discussion

Whilst this does appear to yield the correct results, I am unsure if there is a more performant means to achieve the same output.

Searching for the appropriate vertex arbitrarily choosing either e0.end or e1.start via

let vector = vertices.first { $0.position.isEqual(to: e0.end) } 

to retain the vertex normal, texture coordinates and color seems like a brittle approach. I can not think of an alternative means to tie the translated vertices back to the original vertex - perhaps you can?

As an aside; I note that a dispacement modifier was suggested here but I am unable to find any implementation of Mesh.inset() - has this been removed?

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #106 (600678c) into develop (00a3946) will decrease coverage by 2.54%.
Report is 25 commits behind head on develop.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop     #106      +/-   ##
===========================================
- Coverage    79.55%   77.01%   -2.54%     
===========================================
  Files           30       31       +1     
  Lines         7243     7571     +328     
===========================================
+ Hits          5762     5831      +69     
- Misses        1481     1740     +259     
Files Changed Coverage Δ
Sources/Polygon+CSG.swift 83.09% <0.00%> (-9.09%) ⬇️

... and 16 files with indirect coverage changes

@zilmarinen zilmarinen marked this pull request as draft September 20, 2023 12:26
@nicklockwood
Copy link
Owner

nicklockwood commented Sep 21, 2023

@zilmarinen this method has actually already existed in the develop branch for a while:

https://github.com/nicklockwood/Euclid/blob/develop/Sources/Polygon.swift#L342-L360

I didn't ship it yet because of some edge cases (no pun intended) and because I wasn't sure what to do about texture coordinates (should they be kept the same, or also inset proportionally?). I also wanted to implement equivalent inset methods on Mesh and Path at the same time but those raised even more difficult cases.

I need to refresh my memory about exactly what the issues were, but if this is something you need I'm happy to prioritize getting it into the next release. Any help you can offer in terms of creating test cases and working around the bugs would be appreciated.

@nicklockwood
Copy link
Owner

As an aside; I note that a #83 but I am unable to find any implementation of Mesh.inset() - has this been removed?

Yeah sorry I had temporarily removed the WIP stuff from the develop branch in preparation for next release, so it probably wasn't there when you checked for it.

@nicklockwood nicklockwood force-pushed the develop branch 2 times, most recently from 59f4ce7 to 8d79842 Compare September 23, 2023 22:32
@zilmarinen
Copy link
Contributor Author

Apologies for the delayed response.

I would like to cover this implementation with some unit tests but I am currently at a loss as to what a decent test for this might look like 🤔 Both convex and non-convex shapes need to be considered here as well as positive and negative values for the inset which starts to add a lot of complexity. And this is only for a single polygon. Considering how to implement insetting on meshes blows my tiny mind!

Do you have any considered examples of how this could be tested thoroughly without writing brittle tests that are overly specific to a use case?

@zilmarinen
Copy link
Contributor Author

@nicklockwood Apologies for letting this PR languish. Insetting is still something that I would like to see implemented in Euclid but as it stands, the implementation I have provided here does not fully satisfy all valid use cases.

Your implementation does appear to be more stable - would it be possible to request this is re-added in a future release?

I need to refresh my memory about exactly what the issues were, but if this is something you need I'm happy to prioritize getting it into the next release. Any help you can offer in terms of creating test cases and working around the bugs would be appreciated.

If there are any useful tests or examples I can provide to facilitate this, I am more than happy to assist in moving this forward.

@nicklockwood nicklockwood force-pushed the develop branch 2 times, most recently from 91dd394 to 9643e62 Compare August 7, 2024 11:07
@zilmarinen
Copy link
Contributor Author

Closing this PR as the functionality requested has been added by reintroducing the appropriate .inset() methods for Mesh and Polygon.

@zilmarinen zilmarinen closed this Aug 8, 2024
@zilmarinen zilmarinen deleted the feature/polygon_inset branch August 8, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants