Added a new "Surface" type: ConvexPolygon which support planner simple convex polygons #213
Conversation
… libs, the other one is for fast processing of 2D polygons. Added a new "Surface" child: ConvexPolygon which support planner simple convex polygons. These can be used to create non-symmetric paraxial lenses. Moved a couple of utility functions form the Emitters module to the Geometry module where they belong. These include right, up, forward and origins for type Transform. Also added some tests for the new ConvexPolygon type.
Codecov Report
@@ Coverage Diff @@
## main #213 +/- ##
==========================================
+ Coverage 56.55% 56.69% +0.13%
==========================================
Files 67 68 +1
Lines 6372 6420 +48
==========================================
+ Hits 3604 3640 +36
- Misses 2768 2780 +12
Continue to review full report at Codecov.
|
| struct ConvexPolygon{T} <: Surface{T} | ||
| plane::Plane{T,3} | ||
| local_frame::Transform{T} | ||
| local_points::Vector{Vector{T}} |
There was a problem hiding this comment.
local_points should probably be SVector{SVector}. Vectors will be allocated on the heap which will lead to allocations and will probably also be slower. If we expected large convex polygons then Vector might be appropriate. But this class is going to be used for polygons with less than 20-30 points. If you change to SVector probably should document that should only have small number of points.
There was a problem hiding this comment.
I replaced it with Vector{SVector{2, T}}.
I can't find a convenient way to define an static array of static arrays. Please send me an example if you have one.
There was a problem hiding this comment.
you'd have to do something like this:
struct ConvexPolygon{N,T<:Real} <: Surface{T}
local_points::SVector{N,SVector{2,T}}
or maybe even better
struct ConvexPolygon{Npoints,Dim,T<:Real} <: Surface{T}
local_points::SVector{Npoints,SVector{Dim,T}}
where Dim is the dimension of the points, and Npoints is the number of points.
There was a problem hiding this comment.
@rgal does my suggestion for defined SVector{SVector} work for this case?
There was a problem hiding this comment.
I'm getting the following error when trying it:
ERROR: LoadError: TypeError: in ConvexPolygon, in N, expected N<:Int64, got a value of type Int64.
this is my entire test:
using StaticArrays
struct ConvexPolygon{N<:Int64, T<:Real}
local_points::SVector{N, SVector{2, T}}
function ConvexPolygon(points::SVector{N, SVector{2, T}}) where {N<:Int64, T<:Real}
new{N, T}(points)
end
end
points = SVector(SVector(1.0, 1.0), SVector(2.0, 2.0), SVector(3.0, 3.0))
p = ConvexPolygon{3, Float64}(points)
There was a problem hiding this comment.
You don't want this:
struct ConvexPolygon{N<:Int64,T<:Real}
you want this:
struct ConvexPolygon{N,T<:Real}
The first statement is saying you want N to be a subtype of Int64, which is the same as saying a type equal to Int64 because there are no subtypes of Int64. But the argument you will use in the constructor is a number, not a type.
you'll want to get rid of the N<:Int64 in the ConvexPolygonn constructor as well. I made those two changes and the code worked. This should do it:
struct ConvexPolygon{N, T<:Real}
local_points::SVector{N, SVector{2, T}}
function ConvexPolygon(points::SVector{N, SVector{2, T}}) where {N, T<:Real}
new{N, T}(points)
end
end
| ) where {T<:Real} | ||
|
|
||
| @assert length(local_polygon_points) > 3 # need at least 3 points to define apolygon | ||
| @assert size(local_polygon_points[1])[1] == 2 # check that first point is a 2D point |
There was a problem hiding this comment.
if you use SVector instead of Vector then you can specify vector size variable declaration and can eliminate this assert.
| centroid(poly::ConvexPolygon{T}) where {T<:Real} = poly.plane.pointonplane | ||
| interface(poly::ConvexPolygon{T}) where {T<:Real} = interface(poly.plane) | ||
| normal(poly::ConvexPolygon{T}) where {T<:Real} = normal(poly.plane) | ||
| normal(poly::ConvexPolygon{T}, ::T, ::T) where {T<:Real} = normal(poly) |
There was a problem hiding this comment.
what is this function for? If you really need two then the reason should probably be in a documentation string for the normal function.
There was a problem hiding this comment.
I copied it from the Hexagon example. I think that its just a matter of interface support.
You can have a normal for the surface and a normal for each point on the surface in the case when it's not planer.
There was a problem hiding this comment.
It seems that all the shapes have the same interface - i think that normal without parameters is the one that should be removed but on the other hand what point would you give to a shape that doesn't have a natural parametrization.
| p = point(intersect) | ||
|
|
||
| local_p = world2local(poly.local_frame) * p | ||
| @assert abs(local_p[3]) < 0.00001 # need to find a general epsilon - check that the point lies on the plain |
There was a problem hiding this comment.
Not sure this assert is necessary. If we have a ray/plane intersection then local_p should always be a valid point. An absolute test is subject to problems with scale, if the convex polygons become large for some reason. Could lead to erroneous failures.
| triangles = [] | ||
| for i in 1:l | ||
| p1 = poly.local_points[i] | ||
| p2 = poly.local_points[mod(i,l) + 1] |
There was a problem hiding this comment.
might want to use a variable name other than l here since it looks almost exactly the same as 1 in the code font. Maybe len instead of l?
| end | ||
|
|
||
|
|
||
| function makemesh(poly::ConvexPolygon{T}, ::Int = 0) where {T<:Real} |
There was a problem hiding this comment.
is makemesh used for drawing the convex poly in 3D? Maybe a documentation string as to the purpose of this function.
| @@ -233,6 +234,15 @@ function Transform(rotation::AbstractArray{T,2}, translation::AbstractArray{T,1} | |||
| translation[1], translation[2], translation[3], one(T)) | |||
| end | |||
|
|
|||
|
|
|||
| # define some utility functions | |||
| right(t::Transform{<:Real}) = normalize(Vec3(t[1,1], t[2,1], t[3,1])) | |||
There was a problem hiding this comment.
documentation strings would be helpful here to explain what these functions do and what they are used for. Graphics people will know but optics people might not.
src/utilities.jl
Outdated
|
|
||
|
|
||
| # some place holders for package level function names | ||
| function origin end |
There was a problem hiding this comment.
documentation string would be useful otherwise few people will understand what this function definition is for. Maybe show an example of overloading this function?
…or{2, T}.
Computed the inverse of the local frame once in the constructor instead of inside the ray intersection test - should be more efficient.
|
Depends on what you mean by convenient. SVector{N,SVector{2,T}} where N is whatever the number of 2vectors is. Is this what you were asking or did I misunderstand?
From: Ran Gal ***@***.***>
Sent: Friday, July 2, 2021 5:21 PM
To: microsoft/OpticSim.jl ***@***.***>
Cc: Brian Guenter ***@***.***>; Comment ***@***.***>
Subject: Re: [microsoft/OpticSim.jl] Added a new "Surface" type: ConvexPolygon which support planner simple convex polygons (#213)
@galran commented on this pull request.
________________________________
In src/Geometry/Primitives/NonCSG/ConvexPolygon.jl<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2FOpticSim.jl%2Fpull%2F213%23discussion_r663291194&data=04%7C01%7Cbguenter%40microsoft.com%7Cfc3ae685d2aa4fe8f86b08d93db86e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637608684613633200%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=LUfs%2Ba2bwLV3XbI7FA31ypaglRNIq4d0eoC6gPtRzUA%3D&reserved=0>:
+
+General Convex Polygon surface, not a valid CSG object.
+The rotation of the polygon around its normal is defined by `rotationvec`.
+`rotationvec×surfacenormal` is taken as the vector along the u axis.
+
+```julia
+ConvexPolygon(local_frame::Transform{T}, local_polygon_points::Vector{Vector{T}}, interface::NullOrFresnel{T} = nullinterface(T))
+```
+
+The local frame defines the plane (spans by the right and up vectors) with the plane normal given by the forward vector.
+the local_polygon_points are given with respect to the local frame and are 2D points.
+"""
+struct ConvexPolygon{T} <: Surface{T}
+ plane::Plane{T,3}
+ local_frame::Transform{T}
+ local_points::Vector{Vector{T}}
I replaced it with Vector{SVector{2, T}}.
I can't find a convenient way to define an static array of static arrays. Please send me an example if you have one.
-
You are receiving this because you commented.
Reply to this email directly, view it on GitHub<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2FOpticSim.jl%2Fpull%2F213%23discussion_r663291194&data=04%7C01%7Cbguenter%40microsoft.com%7Cfc3ae685d2aa4fe8f86b08d93db86e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637608684613643195%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=JE1jNrh5uRfy6vgBU7x55XH0aE31h0jevRrkmf9VJfk%3D&reserved=0>, or unsubscribe<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAIWNESXR3NJ7D4KK6L7CU4TTVZJWVANCNFSM47VSTDQQ&data=04%7C01%7Cbguenter%40microsoft.com%7Cfc3ae685d2aa4fe8f86b08d93db86e9e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637608684613643195%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=hY%2BZBNpHNqL1yIS94LdkfFcTRyuiMdxPteviDXRflZ4%3D&reserved=0>.
|
I tried it and could not make it work: -----------------------------------------------------using StaticArrays -----------------------------------------------------resulting in the error: what am i doing wrong here? |
…used to test whether a 2D point lies inside a convex polygon but was using too many allocations during the test. I implemented the actual test and removed all the allocations.
| normal(poly::ConvexPolygon{T}) where {T<:Real} = normal(poly.plane) | ||
| normal(poly::ConvexPolygon{T}, ::T, ::T) where {T<:Real} = normal(poly) | ||
|
|
||
| centroid(poly::ConvexPolygon{N, T}) where {N, T<:Real} = poly.plane.pointonplane |
There was a problem hiding this comment.
Since N and T are not being used on the right hand side of the = you shouldn't need them. Should still generate efficient code if you remove them.
src/Optical/Paraxial.jl
Outdated
| @@ -17,10 +17,10 @@ ParaxialLensConvexPoly(focaldistance, local_frame, local_polygon_points, local_c | |||
| ``` | |||
| """ | |||
| struct ParaxialLens{T} <: Surface{T} | |||
| shape::Union{Rectangle{T},Ellipse{T},Hexagon{T},ConvexPolygon{T}} | |||
There was a problem hiding this comment.
I don't think this will do what you want. I think this will define a ConvexPolygon whose first type argument is T. This might be what you want:
shape::Union{Rectangle{T},Ellipse{T},Hexagon{T},ConvexPolygon{N,T}} where{N}
Then for sure the T type argument to ConvexPolygon is in the right place.
…an SVector{N SVector{2, T}} due to the ripple effects that addition of N in the system.
Currently, this change leads to ParaxielLens which hold a union of shapes iw now referencing the ALL the shape because if the variable nature of ConvexPolygon. Didn't notice a huge difference in speed but we need to be aware of it.
removed the unused T type declaration from some functions.
added two new libraries dependencies - one is part of the Julia basic libs, the other one is for fast processing of 2D polygons.
Added a new "Surface" child: ConvexPolygon which support planner simple convex polygons. These can be used to create non-symmetric paraxial lenses.
Moved a couple of utility functions form the Emitters module to the Geometry module where they belong. These include right, up, forward and origins for type Transform.
Also added some tests for the new ConvexPolygon type.