DependencyGraph Segfault (Default Constructor + GenerateLayers)

[ptheywood]

When a DependencyGraph is constructed using the default constructor, calls to generateLayers(ModelDescription& _model) will trigger a segfault due to a null pointer, as described in #555.

This PR removes the default constructor for DependencyGraph, so that they cannot exist outside of a model.

• Remove DependencyGraph::DependencyGraph()
  • Update relevant tests
• Remove the ModelDescription parameter from DependencyGraph::generateLayers.
  • This may require replacing the member ModelData with a ModelDescription, or moving the required parts of ModelDescription into ModelData.
• Write documentation on expected use and behaviour

Closes #555 (when complete).

[ptheywood]

Decision is to remove the default constructor, enforcing that graphs can only be created via the ModelDescription class. This will make the new test unneccesary.

Swig might need some hinting that the default costructor doesn't exist for it to work (depends on order of %include and forward declarations I believe).

This may just get folded into #551, and this PR closed (to avoid conflicts)

The method generateLayers(ModelDescription& _model) also needs adjusting, as the member should be used rather than the parameter.

[ptheywood]

Some thoughts on this:

flamegpu::ModelDescription have a single, private member variable std::shared_ptr<ModelData> model;.
So it should be trivial / safe to have multiple ModelDescriptions which point to the same shared modelData? I.e. if you have a shared pointer to a ModelData, Getting a ModelDescription should be achievable and trivial via:

    explicit ModelDescription(const std::shared_ptr<ModelData> model_data);
    // ...
    ModelDescription::ModelDescription(const std::shared_ptr<ModelData> model_data) : model(model_data) { } 

This would then allow DependencyGraph to summon the Description from the Data, as required to generate the model.

This will also however require a cahnge to the DependencyGraph class, which currently has:

    const ModelData* model;

This cannot/shouldnot be a pointer to const ModelData, as the ModelData is mutated when the layers are generated.
Currently this only works because the ModelData which is being pointed to is mutated through a non const'd pointer (via the ModelDescription passed into GenerateLayers).

Changing this to be a shared_ptr to the modelData should be all that's required (and tweaking the constructor)

Any thoughts @Robadob @MILeach?

[MILeach]

I didn't like the idea of giving the DependencyGraph an owning pointer to a ModelDescription, so I have instead just removed the dependency on ModelDescription. At this point there is really no need for the DependencyGraph object to even be available to the user directly so I am inclined to move it into a detail namespace/make it publicly unconstructable, with graph diagram printing accessible through the ModelDescription.

[MILeach]

The current API makes the DependencyGraph object directly available to the user, and the user must manually call generateLayers. The proposed API removes direct access to the DependencyGraph. Instead, functionality is exposed to the user via the model object.

Old API

// Fetch the DependencyGraph object
flamegpu::DependencyGraph& dependencyGraph = model.getDependencyGraph();

// Specify dependencies
inputFunction.dependsOn(outputFunction);

// Set execution roots
dependencyGraph.addExecutionRoot(outputFunction);

// Generate the layers
dependencyGraph.generateLayers(model); // Must be called before simulation starts

New API

// Specify dependencies
inputFunction.dependsOn(outputFunction);

// Set execution roots
model.addExecutionRoot(outputFunction);

// Layers are generated automatically

Functionality such as generating a graphic of the DependencyGraph would also now be accessed via the model, e.g. model.generateDOTDiagram(filePath) instead of fetching the DependencyGraph object and then calling dependencyGraph.generateDOTDiagram(filePath).

In effect, this moves the DependencyGraph from being a user facing object to an internal one. The DependencyGraph object is already effectively tied to a single model, as the agent functions it is built from are model specific so I don't see any issue with regards to wanting to apply the same graph to different models etc.

[mondus]

After discussion there should remain an ability to return a const reference to the DependencyGraph as this maybe useful for things like GUI tools.

[MILeach]

Tasks remaining before merge

• Ensure const ref to DependencyGraph can still be obtained via model API
• Check correct methods are const
• Check all required methods have been passed through to model API (print graph, save DOT etc.)
• Check visibility of DG methods
• Check all examples using DG API have been updated (don't forget python)
• Update tests
• Merge master
• Lint
• Run test suite
• Update documentation

[mondus]

Want to retain use of const in CUDASimulate argument. The clone function can instead return a non const which can be modified by building the dependency layers with a const shared pointer to this then stored.

[MILeach]

Want to retain use of const in CUDASimulate argument. The clone function can instead return a non const which can be modified by building the dependency layers with a const shared pointer to this then stored.

Not as trivial as it first seemed. The dependency graph code was written based on certain facts:

• Users had no access to ModelData
• DependencyGraph could only be created through ModelDescription which is non-copyable
• ModelData was only copied once layers had been specified/generated

As such, the dependency relations are specified directly on the AgentFunctionDescription, Submodel and HostFunctionDescription objects. The ModelData object however only holds the corresponding *Data objects, e.g. AgentFunctionData. The DependencyGraph object and *Data objects are cloned when ModelData is cloned, but this is not sufficient information to rebuild the actual dependencies and construct the new layers as the actual dependencies are specified between the Description objects.

I think there are two approaches here we could take:

1. Refactor the DependencyGraph to contain an actual representation of the dependency relations, with agent functions etc. referenced by unique name, i.e. some combination of the agent name & function name. When cloned, the agent functions etc. will now be identifiable without requiring the description object and the graph structure could be rebuilt. In hindsight I should have done this to start with so that the relations and objects weren't so tightly coupled.
2. Dropping the idea of automated generation and returning to the user manually calling generateLayers. This is more consistent with manual layer specification in that a complete ModelDescription with layers generated is used to construct a simulation.

[Robadob]

Cheat then, mark layers as mutable so it can be modified when const. Or modify the gen dep graph to return the layers so you can overwrite them in the cloned model data manually. Either of these will allow you make the genLayers() method const.

Both are imperfect in one way or the other, but seem viable.

If you do the latter, you could make it a private method and give CUDASim/Sim friend access, which fixes the nasty of having a user exposed method which returns a vec of layer descriptions.

[MILeach]

Decision made to go with option 2 for now and open an issue to add automation in the future