-
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
MeshIO using new Properties and PropertyVector. #644
Conversation
"MaterialIDs", MeshLib::MeshItemType::Cell, 1) | ||
); | ||
if (!opt_material_ids) { | ||
WARN("No material ids read."); |
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.
is this possible? and if so, is it still a valid mesh file?
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.
It should not be possible. But ...
Two general things:
this would require a lot of changes to the current read method and i really don't know if it is worth the effort for a legacy interface. i am just suggesting that the current implementation might crash in a number of cases. but it's also okay if we decide not to care about that .... |
Another thing: we currently don't have tests for this. I am not sure if we need some (legacy interface and all that). if you feel like it you could write some ... |
065a20d
to
9897935
Compare
In my opinion this is a legacy file format. I do not want to put much more effort than using the new data structures for properties, i.e. I do not want write test nor I want to improve the robustness. With the new commits I hope I addressed all your other comments. |
elements.push_back(readElement(line_string, nodes)); | ||
std::stringstream ss(line_string); | ||
materials.push_back(readMaterialID(ss)); | ||
elements.push_back(readElement(ss, nodes)); |
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 need to stop reading if readElement()
returns nullptr?
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.
I did not change the already implemented behaviour. What do you think? Opinions of other developers are also wellcome. If we decide to change the behaviour should we do this in a separate PR?
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.
as i mentioned in my comments there are multiple ways to break this read-method. however, in my opinion we an leave it as it is for the most part as it is "only" a legacy interface and ogs5-read methods are similarly buggy (i.e. it is unlikely that someone comes up with a msh-file that will read in ogs5 but not in ogs6).
that said, in this particular case i would also not insert a nullptr but instead either abort reading the mesh or simply skip the element completely. in both cases we can guarantee consistent behaviour afterwards, whereas a nullptr within the element-vector will result in a crash later on...
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.
Changed as requested.
👍 |
9897935
to
e49195f
Compare
MeshIO using new Properties and PropertyVector.
OpenGeoSys development has been moved to GitLab. |
Reading material ids of elements from the old ogs mesh file format (*.msh) into the new data structures.