shinzlet / atomCAD

Mozilla Public License 2.0
0 stars 0 forks source link

Refactor render into two crates (scene and render) #2

Open shinzlet opened 1 year ago

shinzlet commented 1 year ago

Right now, render contains some structs that don't separate responsibilities:

This is a bit confusing in my opinion. The render crate should not be concerned with how AtomCAD decides to group assemblies, parts, or anything else - it should just provide a way to control the rendering of a list of atoms. Additionally, this current organization is confusing to me. It doesn't allow for common CAD features like recursive subassemblies - you cannot import one world into another without just merging their part lists.

I think a more sensible arrangement is:

render crate:

scene crate:

Of course some indirection is needed in the actual implementation, but I think this is a more straightforward interface that does a better job of separating the scene tree from the rendering logic. This also gives us somewhere to store the molecule graph.

shinzlet commented 1 year ago

Currently working on things, just taking some notes here.

Fragment's API is currently built such that you create an Atoms instance externally, then feed it into the Fragment. In the new version of Fragment, which I want to rename Molecule, I think the best API is one where creating a Fragment will automatically create an empty Atoms buffer. Then, as you manipulate the molecule, Atoms is automatically synchronized. This has some issues, i.e. loading from PDB would require frequent buffer reallocation on the GPU - so we will also need a way to pass in an existing atoms buffer. Perhaps we could have a constructor that takes a callback exposing just the atom graph, and once you finish making the atom graph on the CPU, Fragment will create an Atoms buffer out of the Vertices of the graph?

shinzlet commented 1 year ago

Currently, each Fragment has a transform on the GPU, and inside of the renderer, fragment IDs and a hash map are used to associate each fragment with its transform ID in the gpu buffer. This introduces an unwanted interdependency between scene and render - the concept of a Fragment/Molecule should not exist in the atom renderer.

I'm already reworking the renderer so that instead of taking and using the World API, it simply accepts an iterator of Atoms buffers that it should render.

The most sensible decision from an API perspective is just to have that iterator of atoms instead become an iterator where Item = (&'a Atoms, &'a ultraviolet::Mat4) - basically, the world just exposes the gpu handles to the atom buffers and the transform of each.

At first glance I was worried about this - it would mean that we are uploading all of the transforms to the GPU (1 per molecule) every time we render a frame. However, we are already passing the transform ID of every molecule into the GPU each frame. Assuming a massive scene has 10000 molecules in it, the bandwidth for transform data changes as follows: old: 1 u64/molecule/frame -> 80000 bytes / frame = 8kb/frame -> 480kb/s (assuming 60fps) new: 16 f64 /molecule/frame (i.e. sending a 4x4 float matrix) -> 7.68mb/s (assuming 60fps) This is a significant change - we're using 16x the bandwidth as before for the new architecture - but overall it is very small compared to the bandwidth of PCIE.

After these considerations, (and asking around - it sounds that uploading transforms every frame is common) I'm going to continue refactoring this way.

If this becomes a problem in the future, it would not require a significant refactor to add the transform into the Atoms buffer header, use instancing to reduce overhead, or even implement a system similar to the one in place right now (but with better separation of concerns).

shinzlet commented 1 year ago

I've got render/src/lib.rs and render/passes/molecular.rs ported to the new way of doing things, I think - I need to remove many of the FragmentID references to get things building, though.

shinzlet commented 1 year ago

Got the new architecture to run successfully with cc4be77 - this also fixes #1, indicating that the issue there was likely caused by the Atoms BufferVec header size not being properly accounted for. This indicates that the header size is causing the off by one error I noticed before, so the workaround in #1 isn't sufficient

shinzlet commented 1 year ago

Trying to figure out how best to synchronize the Molecule data with the gpu. The naive approach is to just stuff Atoms inside of Molecule, and then update the GPU buffer every time there is an operation. This however also breaks separation of concerns - all of a sudden, the scene is very concerned with the state of the GPU.

A way around this is to reupload the atoms to the GPU each frame. However this is very costly, and in almost 100% of frames, there is no change.

I think a good way to work around this would be to ignore the Atoms buffer in Molecule. Then, the main logic will check the last edited time of each molecule, and only reupload its data to Atoms if there was a change. This scheme could be improved by fragmenting each molecule into chunks of say 100 atoms, and then only reuploading the altered chunks (but this is very complex).

Not sure what route to take, but wanted to put some notes down here.

shinzlet commented 1 year ago

I'm still unsure what the best route is long-term. Another option I had not considered is to make a new struct, for example AssemblyRenderer, which stores an assembly and a vec of Atoms buffers, and then assembly_renderer.render(&mut renderer) can be used to manage everything cleanly under one interface (using the reupload-on-change model I discussed).

None of these solutions seem very well organized, however - for the time being, I am going to implement reupload-on-change Atoms management in Molecule itself, because it is easy to move that code somewhere else at a later time.