nicklockwood / Euclid

A Swift library for creating and manipulating 3D geometry
MIT License
644 stars 53 forks source link

Add method to inset polygon vertices #106

Closed zilmarinen closed 3 months ago

zilmarinen commented 1 year ago

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

Checklist:

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[bot] commented 1 year ago

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%) :arrow_down:

... and 16 files with indirect coverage changes

nicklockwood commented 1 year ago

@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 commented 1 year ago

As an aside; I note that a https://github.com/nicklockwood/Euclid/issues/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.

zilmarinen commented 1 year ago

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 commented 5 months ago

@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.

zilmarinen commented 3 months ago

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