Switch to new emitter code #140
Conversation
alfredclwong
added
documentation
alfredclwong
force-pushed
the
new-emitter
branch
from
e930061 to
68fead3
Compare
modified Examples.jl drawSchmidth to use new emitters.
renamed Emitter.jl to OldEmitter.jl to find all places where old Emitter code is used. moved AbstractRayGenerator types from Emitter.jl to RayGenerator.jl fixed Examples.drawSchmidt so it draws correctly using the new emitter classes. merge with AbstractOpticalSystem changes
alfredclwong
force-pushed
the
new-emitter
branch
from
68fead3 to
3046b1b
Compare
Codecov Report
@@ Coverage Diff @@
## main #140 +/- ##
==========================================
+ Coverage 48.37% 49.81% +1.44%
==========================================
Files 65 64 -1
Lines 6667 6424 -243
==========================================
- Hits 3225 3200 -25
+ Misses 3442 3224 -218
Continue to review full report at Codecov.
|
alfredclwong
force-pushed
the
new-emitter
branch
from
9aad498 to
55223a1
Compare
alfredclwong
force-pushed
the
new-emitter
branch
from
c1c4a38 to
41a4864
Compare
alfredclwong
force-pushed
the
new-emitter
branch
from
7d067fe to
1f58cd5
Compare
|
@alfredclwong this looks great. Is it ready to go from draft to regular PR? I'm seeing changes you have made that I have also made on another branch and would like to merge your PR into main as soon as possible so I can sync with your version. |
Thanks! It's pretty much ready to go, I just wanted to have a crack at syntax highlighting and to get some feedback on the new docs using |
|
Yeah looks great @alfredclwong! Syntax highlighting would be nice as you say, but already this makes life a lot easier - let's make sure to remove all commented code before merging |
|
It would be nice to be able to colour the source surfaces, and perhaps have the default be something different from the ordinary default surface colour so they are distinct? Can't quite follow where/how these are actually getting drawn now with all the diffs and comments |
I looked into colouring surfaces a while back because I was working with waveguides which kept being drawn out in very distracting rainbow-like patterns! We had a discussion about how to implement it, and I think Ran wrote some code, but I can't remember it being added to the main project. I'll revisit it later. |
alfredclwong
force-pushed
the
new-emitter
branch
from
720e1a9 to
4f12d31
Compare
alfredclwong
force-pushed
the
new-emitter
branch
from
bca0609 to
6a964e7
Compare
|
Syntax highlighting is working! See the The code still needs reshuffling to follow a single convention (see #144, I agree with Charlie). I'm pretty busy this weekend so it'll be a while until I can sit down and code - should be quite easy work though so anyone can do it if you have the time. |
The duplicate code (there's not much) is on the optimization branch. |
alfredclwong
force-pushed
the
new-emitter
branch
2 times, most recently
from
aa30b60 to
a787732
Compare
this means that all we need to do in examples.md is show the (syntax-highlighted) code_string and then call the corresponding draw_* function in an @example block also consolidated the usage of Maybe{T} and MaybeVector{T} within OpticSim by definining them within OpticSim.jl (sources.jl still has to define it separately because it is included directly in deps/build.jl) fixes #144
alfredclwong
force-pushed
the
new-emitter
branch
from
a787732 to
688ecc6
Compare
alfredclwong
changed the title
| @@ -18,7 +19,8 @@ using Revise | |||
| import GLMakie | |||
| import Makie.AbstractPlotting | |||
|
|
|||
| unzip(a) = map(x -> getfield.(a, x), fieldnames(eltype(a))) | |||
There was a problem hiding this comment.
is this ok to remove?
There was a problem hiding this comment.
not used anywhere else in the code so ok to remove.
| function Constant(dirx::T,diry::T,dirz::T) where{T<:Real} | ||
| return new{T}(Vec3(dirx,diry,dirz)) | ||
| end | ||
|
|
There was a problem hiding this comment.
@BrianGun this is copied this from the optimization branch so there shouldn't be any merge conflicts.
| @@ -236,6 +236,7 @@ function save(path::String) | |||
| AbstractPlotting.resize!(current_main_scene, size) | |||
| display(current_main_scene) | |||
| end | |||
| function save(::Nothing) end | |||
There was a problem hiding this comment.
for convenience (instead of checking if isnothing(filename))
| """ | ||
| draw!(scene::MakieLayout.LScene, origingenerator::RayOriginGenerator; kwargs...) | ||
|
|
||
| Draw the surface representing `origingenerator`. | ||
| """ | ||
| function draw!(scene::MakieLayout.LScene, origingenerator::Union{RandomRectOriginPoints{T},GridRectOriginPoints{T}}; position::SVector{3,T} = SVector{3,T}(0, 0, 0), color = :white, kwargs...) where {T<:Real} | ||
| draw!(scene, Rectangle(Plane(origingenerator.direction, origingenerator.position + position), origingenerator.halfsizeu, origingenerator.halfsizev, origingenerator.uvec, origingenerator.vvec); color = color) | ||
| end | ||
|
|
||
| function draw!(scene::MakieLayout.LScene, origingenerator::Union{HexapolarOriginPoints{T},RandomEllipseOriginPoints{T}}; position::SVector{3,T} = SVector{3,T}(0, 0, 0), color = :white, kwargs...) where {T<:Real} | ||
| draw!(scene, Ellipse(Plane(origingenerator.direction, origingenerator.position + position), origingenerator.halfsizeu, origingenerator.halfsizev, origingenerator.uvec, origingenerator.vvec); color = color) | ||
| end | ||
|
|
||
| function draw!(scene::MakieLayout.LScene, origingenerator::OriginPoint{T}; position::SVector{3,T} = SVector{3,T}(0, 0, 0), color = :white, kwargs...) where {T<:Real} | ||
| draw!(scene, origingenerator.position + position; color = color) | ||
| end | ||
|
|
||
| """ | ||
| draw!(scene::MakieLayout.LScene, raygen::OpticalRayGenerator, norays::Bool = false; kwargs...) | ||
|
|
||
| Draw the surface representing `raygen`, as well as some sample [`OpticalRay`](@ref)s eminating from it (providing `norays` is false). | ||
| """ | ||
| function draw!(scene::MakieLayout.LScene, raygen::OpticalRayGenerator{T}; position::SVector{3,T} = SVector{3,T}(0, 0, 0), norays::Bool = false, kwargs...) where {T<:Real} | ||
| if !norays | ||
| for r in raygen | ||
| draw!(scene, OpticalRay(origin(r) + position, direction(r), power(r), wavelength(r), opl = pathlength(r), nhits = nhits(r), sourcenum = sourcenum(r)); kwargs...) | ||
| end | ||
| end | ||
| if hasproperty(raygen, :sourcenum) | ||
| col = indexedcolor(raygen.sourcenum) | ||
| else | ||
| col = :white | ||
| end | ||
| if !(nothing === origingen(raygen)) | ||
| draw!(scene, origingen(raygen), position = position, color = col) | ||
| end | ||
| end | ||
|
|
||
| """ | ||
| draw!(scene::MakieLayout.LScene, raygen::GeometricRayGenerator, norays::Bool = false; kwargs...) | ||
|
|
||
| Draw the surface representing `raygen`, as well as some sample [`Ray`](@ref)s eminating from it (providing `norays` is false). | ||
| """ | ||
| function draw!(scene::MakieLayout.LScene, raygen::GeometricRayGenerator{T}; position::SVector{3,T} = SVector{3,T}(0, 0, 0), norays::Bool = false, kwargs...) where {T<:Real} | ||
| if !norays | ||
| for r in raygen | ||
| draw!(scene, Ray(origin(r) + position, direction(r)); kwargs...) | ||
| end | ||
| end | ||
| draw!(scene, origingen(raygen), position = position, color = :white) | ||
| end | ||
|
|
||
| function draw!(scene::MakieLayout.LScene, pixel::PixelSource{T,C}; kwargs...) where {T<:Real,C} | ||
| for (i, raygen) in enumerate(pixel.subpixels) | ||
| draw!(scene, raygen; kwargs...) | ||
| end | ||
| end | ||
|
|
||
| function draw!(scene::MakieLayout.LScene, display::BasicDisplayPanel{T,C}; kwargs...) where {T<:Real,C} | ||
| draw!(scene, display.generator; kwargs...) | ||
| end | ||
|
|
||
| function draw!(scene::MakieLayout.LScene, arr::OpticalSourceArray{T}; position::SVector{3,T} = SVector{3,T}(0, 0, 0), kwargs...) where {T<:Real} | ||
| for pos in arr.positions | ||
| draw!(scene, arr.generator; kwargs..., position = pos + position) | ||
| end | ||
| end | ||
|
|
||
| function draw!(scene::MakieLayout.LScene, arr::OpticalSourceGroup{T}; kwargs...) where {T<:Real} | ||
| for gen in arr.generators | ||
| draw!(scene, gen; kwargs...) | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
is it ok to remove these?
| rays = RayListSource([OpticalRay([0.0,0.0,0.0],[0.0,0.0,1.0],1.0,.78) for _ in 1:100]) | ||
| trace(conv,rays) | ||
| rays = Emitters.Sources.CompositeSource(translation(-unitZ3()), repeat([Emitters.Sources.Source()], 100)) | ||
| trace(conv, rays) |
There was a problem hiding this comment.
does this still fulfil the original purpose of this test?
| using OpticSim, StaticArrays # hide | ||
| Vis.draw(CosineOpticalSource(RandomSource(OriginPoint{Float64}(200, direction = SVector(0.0, 0.0, 1.0))), 1.0, 0.45)) | ||
| Vis.save("assets/source3.png") # hide | ||
| using OpticSim, OpticSim.Geometry, OpticSim.Emitters # hide |
There was a problem hiding this comment.
Does this new example do the same thing as the old one? It doesn't matter so long as the new text matches the new example.
Examples preview
Changes