Closed ndinsmore closed 1 year ago
Thank-you very much. It looks really interesting.
unitize
was not used anywhere test/runtests.jl
. It is because the conditions with the return of intersectAABoxRay(...)
needs to be adjusted. I fixed the issue wit the test. There are two left failing because the distout is 2.09e-9 and the failure criteria is < 1e-9/2
That said a large part of the improvement has disappeared. This is still some but it is only like 15%-20% now
The following implementation of distanceToIn(box::Box{T}...)
fixes all the tests. I have not observed a nan
.
function distanceToIn(box::Box{T}, point::Point3{T}, direction::Vector3{T},rcp_direction::Vector3{T}=inv.(direction))::T where T<:AbstractFloat
max = Point3{T}(box.fDimensions)
(distin, distout) = intersectAABoxRay(-max, max, point, direction, rcp_direction)
(distin >= distout || distin <= -kTolerance(T)) ? Inf : distin
end
However I still have problem generating the X-ray in the z direction.
What was the edge case that the surface checks were trying to fix? I noticed that there was not something similar VecGeom. I believe that the new method is effectively doing what the previous methods were, this is just a streamlined slabs, so i don't know how that could effect the z direction.
Also do you have any opinion on the naming of intersectAABoxRay
it was originally intersectAABBRay
but I thought that was to close the AABB type.
The isnan
is important because it will happen for 0*Inf
then when you do a comparison to the nan it returns false. The fixed version I posted that has isnan was important for that reason.
Yes, you are right the handling of NaN
is very important. But this way produces the wrong result. See for example a unit Box
and a Point
in one of the vertices.
Geom4hep.distanceToIn(Box{T}(1,1,1), Point3{T}(-1,-1,-1),Vector3{T}(0,1,0))
The result should be 0
since you are already in the surface of the Box
while it returns Inf
. The problem is that the functions min()
and max()
do not handle correctly the NaN
case.
Should work now. There is an interesting post about all the floating point logic at: https://tavianator.com/2022/ray_box_boundary.html
Yes, it does work now. Thanks for the pointer to the post. I can obtain the same X-ray images. However the speedup has vanished or almost vanished. Did you measure it again with this latest version?
Yeah I was still getting about 10% 2.2s -> 2s
I would not merge this anymore just merge #6 instead of this it includes it and actually results in a performance benefit
Supplemented by #6
First, the bug fix is to make
unitize
return a vector with a magnitude of 1.I thought this would just be a micro-optimization but it ends up being fairly significant.
There are two caveats to this improvement:
distsurf
, which I could not really figure out what it consistently calculated so I was unable to implement it. I don't think that should change the results much.Running the benchmark:
Master:
This PR: