Occlusion Handler #109
Occlusion Handler #109
Conversation
DRVeyl
force-pushed
the
OcclusionHandler
branch
from
8cbc24d to
2d802b2
Compare
dkavolis
force-pushed
the
master
branch
2 times, most recently
from
75bcb75 to
5f0e911
Compare
| public struct SetVectorsJob : IJobParallelFor | ||
| { | ||
| [ReadOnly] public NativeArray<quaternion> quaternions; | ||
| [ReadOnly] public float4x4 localToWorldMatrix; |
There was a problem hiding this comment.
RigidTransform may be more appropriate than float4x4 for translation + rotation
There was a problem hiding this comment.
Not sure what to do here. Maybe, but the localToWorldMatrix is easily passed from outside. Maybe I'll get an improvement, but SetVectorsJob [now as modified] consumes a very small fraction of the compute here. (Current run on my PC -- maybe not the most useful reference without a lot more context -- is 0.028ms)
| public void Execute(int index) | ||
| { | ||
| float3 rot_dir = math.mul(rotations[index], new float3(0, 0, 1)); | ||
| angles[index] = math.degrees(math.acos(math.min(math.max(math.dot(dir, rot_dir), -1f), 1f))); |
There was a problem hiding this comment.
math.clamp instead of min and max and is degrees really needed here when all trigonometric functions work with radians?
There was a problem hiding this comment.
Partially Implemented in 53809f6. Left as degrees, pending reviewing how it gets used outside of the job. (I'm certain I thought about this, and it's probably because some of the Physics calcs from RA use degrees. So does..., much of Unity that isn't in Unity.Mathematics?)
| * */ | ||
| namespace FerramAerospaceResearch.FARAeroComponents | ||
| { | ||
| public class VehicleOcclusion : MonoBehaviour |
There was a problem hiding this comment.
Is there a reason this is a MonoBehaviour and not VesselModule? A module can be found without using GetComponent which has to call a native method. FARVesselAero is basically never used and it's lifetime is bound to Vessel anyway. Removing the coupling between the two would reduce complexity
Also, MFI uses [DefaultExecutionOrder(10)] so it might be worth setting this class to [DefaultExecutionOrder(9)] to give it more time for jobs
There was a problem hiding this comment.
Some reason. Yes, I could make it a VesselModule. I don't think that is more efficient than using Unity's GetComponent method, since that's the normal way Unity expects things. However, I cannot completely decouple from FARVesselAero because we require a notification every time voxelization completes. Maybe I could register with something else and get that notification another way? But that seemed the right kind of place. So some coupling has to remain, though becoming a VesselModule would remove the need to get created in FARVesselAero.
VehicleOcclusion doesn't need to survive outside of a Vessel, either.
| bool OnValidPhysicsVessel => farVesselAero && Vessel && (!Vessel.packed || Vessel == FlightIntegrator.ActiveVesselFI?.Vessel); | ||
|
|
||
| private readonly Dictionary<Transform, Part> partsByTransform = new Dictionary<Transform, Part>(); | ||
| private readonly Dictionary<Part, DirectionalOcclusionInfo> partOcclusionInfo = new Dictionary<Part, DirectionalOcclusionInfo>(); |
There was a problem hiding this comment.
Might be a good idea to use an explicit equality comparer such as reference or one using Object.GetInstanceID for hash for both dictionaries
| FARLogger.Info($"[VehicleOcclusion] {Vessel?.name} Learned Center {center} and Extents {extents}"); | ||
| } | ||
|
|
||
| private void DisposeLongTermAllocations() |
There was a problem hiding this comment.
I don't understand what to do here, as the scope for these is not something I can place into a try/finally, or into a using statement as I understand it.
There was a problem hiding this comment.
It's just changing the method name to Dispose, scope doesn't matter but IDisposable shows that the class has resources that need to be cleaned up
| @@ -21,7 +21,7 @@ | |||
| <DebugType>portable</DebugType> | |||
| <Optimize>false</Optimize> | |||
| <OutputPath>$(SolutionDir)bin\$(Configuration)\</OutputPath> | |||
| <DefineConstants>DEBUG;TRACE;ASSERT;LOG_TRACE</DefineConstants> | |||
| <DefineConstants>DEBUG;TRACE;ASSERT;LOG_TRACE;ENABLE_PROFILER</DefineConstants> | |||
There was a problem hiding this comment.
Could you add a new configuration for profiling? Not every debug build run needs a profiler
| if (state == State.Running && jobsInProgress == 0 && processedQuaternionsMap.Length == Quaternions.Length) | ||
| state = State.Completed; | ||
| else if (state == State.Running) | ||
| HandleJobCompletion(raycastJobTracker, UpdateMetric); |
There was a problem hiding this comment.
Does this need to run in LateUpdate when the calculations only affect physics?
There was a problem hiding this comment.
Yes. The goal is to actually do most of the compute to fill in the cache during the Update cycle, and only do things in FixedUpdate that will be required immediately and weren't already computed. So I'm trying to allow as much time as possible during the Update portion, by collecting the results in LateUpdate.
Default to stock drag cube handling until either of these systems is ready, ie voxelization has completed.
Manually merge Voxelization and Occlusion edits
Precompute some constants Unity.Burst.CompilerServices.Hint.Unlikely Unity.Mathematics trig methods, type conversions and overloads Extract some computations out of loops
Remove duplicate Voxelization node Comment settings
Language features and fix naming rule
Make Lattice_Epsilon constant. It varies with lattice size for some purposes, but for the distribution we want, it does not. See http://extremelearning.com.au/how-to-evenly-distribute-points-on-a-sphere-more-effectively-than-the-canonical-fibonacci-lattice/
dkavolis
force-pushed
the
master
branch
14 times, most recently
from
dae4da0 to
a672624
Compare
dkavolis
force-pushed
the
master
branch
2 times, most recently
from
a556f26 to
df51dd9
Compare
Unity Jobs+Burst-based Occlusion Handling for FlightIntegrator
Precompute occlusion from configurable number of directions on a sphere (currently 2000), sample by raycasting over the entire vessel and record per-part dictionary of <direction, area of total raycasts hitting collider>. Limit calculations during FixedUpdate cycle to only those that will be required for physics on that pass. Consume configured number of additional directions during Update() cycle, prioritized by near-ness to current vessel orientation. (That is, attempt to predict which orientations not yet calculated are most likely to be requested next.)
Since it is a physics raycaster, it can only hit colliders. Badly-configured part colliders, or missing colliders, will have an impact. (No collider ala ProcFairings [pre-1.8.3] means no detected surface area/occlusion for thermal processing.)
For testing / to exercise more fully, re-calculates over entire vessel [configurable interval, default 180 seconds] after completion, or whenever voxelization completes. Ideally the precomputation is a one-time cost each time the vessel changes. Part animations, if they move colliders, should trigger a recompute. Things in the environment can also occlude, so decoupling two vessels may lead to one occluding the other, and this state should get re-computed ... some time later. TBD if this is a feature or a bug.
Known issue: Update-cycle jobs cap is per-vessel instead of global. If multiple vessels are in physics range (such as after decoupling a lot of fairingsides) then they will all consume their allotment of raycast jobs. Need to track how many total jobs have been created across all active vessels, not per-instance. Probably should prioritize access by the ActiveVessel in this instance? Does it matter if we starve some vessels of their pre-compute jobs, or should all vessels get a minimum?
Implementation note: Current raycaster is limited to 10,000 rays over entire vessel. Min interval between rays is 0.1m in each dimension. Vessel dimension <10m will cast fewer rays. Vessel dimension > 10m will stretch the interval in that dimension to cast 100 rays along that slice. Not sure if I should keep it this way with the fixed min interval, or configure the raycaster to always shoot 10,000 rays and not have a minimum interval (so a small part/vessel will get a more precise area measurement). Forcing all casts to the fixed size would cost some performance in exchange for some accuracy, not sure if that is needed.