Add some updates as per JuliaCon Paper Review #13
Conversation
Codecov Report
@@ Coverage Diff @@
## juliacon-paper #13 +/- ##
==================================================
- Coverage 47.29% 46.85% -0.44%
==================================================
Files 18 18
Lines 757 764 +7
==================================================
Hits 358 358
- Misses 399 406 +7
Continue to review full report at Codecov.
|
| rendering by a significant amount. The main design behind an acceleration | ||
| structure should be such that it can be used as a simple replacement for | ||
| a vector of [`Object`](@ref)s. | ||
| """ | ||
| abstract type AccelerationStructure end |
There was a problem hiding this comment.
I'm just wondering if this type is really needed (i.e., would adding methods that have a specific implementation for all acceleration structures be relevant?). It might still be useful if you plan on drawing the type hierarchy, for instance, so I'm not proposing to remove it (unless you think it makes things clearer).
There was a problem hiding this comment.
would adding methods that have a specific implementation for all acceleration structures be relevant?
No, we can't have a specific implementation relevant to all acc. structures
This type is simply so that we have a clean type hierarchy.
src/bvh.jl
Outdated
| """ | ||
| BoundingVolumeHierarchy | ||
|
|
||
| Construct the Bounding Volume Hierarchy from a given Vector of |
There was a problem hiding this comment.
You're documenting a type, not a function, so you can't say that you "construct the BVH". "A BVH hierarchy"?
This piece of documentation might be moved to line 63 (function BoundingVolumeHierarchy(scene::Vector)).
(BTW, why are you using an erased type Vector? Can't you be more precise, like Vector{<:Triangle}?)
| @@ -28,5 +35,10 @@ end | |||
| zeroonenorm(x::AbstractArray) | |||
|
|
|||
| Normalizes the elements of the array to values between `0` and `1`. | |||
|
|
|||
There was a problem hiding this comment.
Shouldn't you then remove this workaround and upgrade Zygote in your Project.toml?
There was a problem hiding this comment.
The main issue is there are some blocking issues on Zygote's end that prevent me from updating it. Even if it is easy to workaround on the end of RayTracer, those changes cause segfault in case of Duckietown.
|
I am merging this PR for now. Will update the test suite in a later PR. |
Changes introduced:
Things to do before merging: