Configure gradient data requirement automatically #1371
Conversation
davidscn
added
enhancement
There was a problem hiding this comment.
Please try to avoid long call chains. This makes it hard to change the code and leads to spaghettification ( https://en.wikipedia.org/wiki/Law_of_Demeter).
| @@ -86,7 +86,7 @@ void MeshConfiguration::xmlTagCallback( | |||
| bool found = false; | |||
| for (const DataConfiguration::ConfiguredData &data : _dataConfig->data()) { | |||
| if (data.name == name) { | |||
| _meshes.back()->createData(data.name, data.dimensions, _dataIDManager.getFreeID(), data.hasGradient); | |||
| _meshes.back()->createData(data.name, data.dimensions, _dataIDManager.getFreeID()); | |||
There was a problem hiding this comment.
I know, this is not your design, but this is difficult to understand. Why is _meshes a vector? I guess only the last Mesh _meshes.back() is the "active" one where data is created. Creating a function currentMesh() that only calls _meshes.back() would make this easier to read. This would also allow us to change the underlying datastructure (not sure if this is really necessary, but to me the vector looks not fitting).
There was a problem hiding this comment.
I quickly skimmed over the places where _meshes or meshes() are used and overall I have the impression that at most places replacing the vector with a dict would help. One could also introduce some more high-level functions in MeshConfiguration to avoid call-chains like meshConfig->meshes().at(0)->createVertex(Eigen::Vector3d(1.0, 1.0, 1.0)); in the client code.
There was a problem hiding this comment.
What is a dict? Do you mean map? I agree that there are several places, which are just confusing. If we can boil down 'problem classes' we should open an issue and discuss how to adapt things. I would prefer to avoid adding such changes in this PR.
davidscn
force-pushed
the
auto-conf-grads
branch
from
0914c07
to
d496a3b
Compare
Co-authored-by: Benjamin Uekermann <benjamin.uekermann@gmail.com>
Main changes of this PR
Removes the
<data: ... gradient="on" />flag from the xml configuration and deduces the information automatically based on the configured mappings. Currently, only thenearest-neighbor-gradient-mappingrequires gradient data.Motivation and additional information
Improves the usability, as the user doesn't have to change obvious things in different places and prevents unnecessary gradient data allocation due to global gradient enabling/disabling. Related to #1252.
Note that this change is non-breaking as this feature is still experimental.
EDIT: This also resolved part two of #1252, i.e., gradients are now only communicated if it is really required by the mapping configuration.
Author's checklist
make changelogif there are user-observable changes since the last release.make formatto ensure everything is formatted correctly.Reviewers' checklist