-
Notifications
You must be signed in to change notification settings - Fork 239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Break fileIO ApplicationsLib dependency #1138
Break fileIO ApplicationsLib dependency #1138
Conversation
7cf6b60
to
f044d58
Compare
Jenkins: OGS-6/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Win-PRs/1449/ |
Jenkins: OGS-6/Linux-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Linux-PRs/1565/ |
Jenkins: OGS-6/Gui/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Gui/job/Win-PRs/1426/ |
XmlGspInterface::XmlGspInterface( | ||
GeoLib::GEOObjects& geoObjects, | ||
std::vector<MeshLib::Mesh*> const& mesh_vector, | ||
std::function<void(MeshLib::Mesh* const)> add_mesh_callback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move it: &&
✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prevents the Data Explorer from loading anything except geometries and meshes from a project?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it does not. It is only separating circular dependency of the FileIO and ApplicationsLib. The code for reading is not touched, so everything what was read should behave exactly the same. Only the place where the mesh is added to project has moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if the project file reader has no knowledge of ProjectData but only of GeoObjects + the mesh vector it can only store the related information. I don't doubt that everything works fine at the moment but the next step after reorganising the project file reader would be reading/writing boundary conditions which would then require another parameter, I think? Maybe we could talk about this tomorrow or on friday? (maybe together with Tom as would probably be involved in the next stage)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rinkk, I can understand your concerns. But if we keep all parts of ProjectData
in here, soon FileIO
will also depend on ProcessLib
(for BCs). Probably that shouldn't be.
Maybe a different approach would be to move some of the parsing code to the DE directory. Do you think, that's feasible?
E.g., the following case checking code probably should be done in some DE class.
if (file_node.compare("geo") == 0)
Maybe at some point one could even use the constructor that is provided with ProjectData
to read the *.prj
(if we remove std::abort()
😄).
That's at least what I can say after a discussion with @endJunction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it's not good idea to mix core parts and application layers together. How about provide separate FileIO for ApplicationLib? Current FileIO can be used to provide read and write of basic data, e.g. geo and mesh.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Nori. If planning a major cleanup of the FileIO/ApplicationsLib separation of the business logic and the actual reading of the data would be a good goal.
We shall have a 30min discussion tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or to make it consistent for all libs, we can create FileIo for each lib, e.g. geolib/fileio, meshlib/fileio
Last commit (f044d58) looks good. Just make it compile. 👍 |
As a general question: Why does the DataExplorer need to be decoupled from FileIO? |
@@ -153,8 +156,9 @@ bool XmlGspInterface::write() | |||
} | |||
|
|||
// MSH | |||
const std::vector<MeshLib::Mesh*> msh_vec = _project.getMeshObjects(); | |||
for (std::vector<MeshLib::Mesh*>::const_iterator it(msh_vec.begin()); it != msh_vec.end(); ++it) | |||
for (std::vector<MeshLib::Mesh*>::const_iterator it(_mesh_vector.begin()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not shorter and more readable: for (auto const* mesh : _mesh_vector)
? ✅
The data explorer is not decoupled from FileIO, and actually nothing is decoupled. The circular dependency of ApplicationLib and FileIO is removed. |
Jenkins: OGS-6/Gui/Mac-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Gui/job/Mac-PRs/1433/ |
Jenkins: OGS-6/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Win-PRs/1454/ |
Jenkins: OGS-6/Gui/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Gui/job/Win-PRs/1431/ |
Jenkins: OGS-6/Gui/Gui-Linux-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Gui/job/Gui-Linux-PRs/1540/ |
Jenkins: OGS-6/Linux-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Linux-PRs/1570/ |
Jenkins: OGS-6/Mac-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Mac-PRs/1451/ |
Jenkins test this please. |
Jenkins: OGS-6/Gui/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Gui/job/Win-PRs/1432/ |
Jenkins: OGS-6/Gui/Gui-Linux-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Gui/job/Gui-Linux-PRs/1541/ |
Jenkins: OGS-6/Gui/Mac-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Gui/job/Mac-PRs/1434/ |
Jenkins: OGS-6/Linux-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Linux-PRs/1571/ |
Jenkins: OGS-6/Win-PRs failed: https://svn.ufz.de:8443/job/OGS-6/job/Win-PRs/1455/ |
Would it help to create a seperate data holder object for the Data Explorer in, e.g. BaseLib, and keep everything else as it is? |
In the discussion we came to the following two steps:
|
) | ||
|
||
if(METIS_FOUND) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TOdo for later, not now: we shouldnt always link metis and lapack just because Cmake could find them. there should be a flag like OGS_USE_METIS.
Noted in https://github.com/ufz/ogs/issues/1142
i haven't checked ProjectData stuff, but other changes look nice. just a question: you removed logog dependency from most of libs. is it because BaseLib depends on it? |
The logog is removed as dependency from most libs but BaseLib, because the default behaviour for linking is transitive. See cmake doc. I'm yet figuring out, what the minimum specification is, such that all links fine w/o overhead. But first resolving the circular dependencies bugs. |
thanks for the explanation! |
f14124d
to
91dfd17
Compare
Update: Initially I thought the EnableSharedLibs PR would be much easier, but first I need to break the circular deps. So this is now a stand-alone PR not based on shared libs but set up as a prerequisite for them. |
Build finished. |
👍 |
Adding a callback for adding project meshes and pass additional arguments to XmlGspInterface constructor.
91dfd17
to
00c37bc
Compare
OpenGeoSys development has been moved to GitLab. |
Follow up of #1133.This PR adds only the one commit [DE] Break ApplicationLib dependency.The dependency is caused by usage of ProjectData::addMesh by XmlGspInterface.
Unfortunately the programming of that class and its parents, the XMLInterface and XMLQtInterface is not as separate as it could be, therefore the current solution can be seen as a quickfix.
Clean solution would be, for example, a separation of the reader and the writer parts of the XMLInterface(s) and to use standalone functions for qt's xml validation.