Skip to content
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

Mike21 Driver #470

Merged
merged 25 commits into from
Nov 24, 2023
Merged

Mike21 Driver #470

merged 25 commits into from
Nov 24, 2023

Conversation

JanCaha
Copy link
Contributor

@JanCaha JanCaha commented Nov 22, 2023

Driver for Mike21 format: https://www.xmswiki.com/wiki/SMS:MIKE_21_*.mesh.

Currently there is some code duplication with 2DM format, which could be eliminated by making functions _parse_vertex_id_gaps() and _persist_native_index() part of MDAL API. Also the class that extends MemoryMesh to store vertexIDs that might not be continuous could be generalized and shared by Mike21 and 2DM format.

Copy link
Contributor

@PeterPetrik PeterPetrik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

tests/test_mike21.cpp Outdated Show resolved Hide resolved
mdal/frmts/mdal_mike21.hpp Outdated Show resolved Hide resolved
mdal/frmts/mdal_mike21.hpp Outdated Show resolved Hide resolved
mdal/frmts/mdal_mike21.hpp Outdated Show resolved Hide resolved
mdal/frmts/mdal_mike21.cpp Outdated Show resolved Hide resolved
mdal/frmts/mdal_mike21.cpp Show resolved Hide resolved
@PeterPetrik
Copy link
Contributor

_parse_vertex_id_gaps() and _persist_native_index() part of MDAL API

definitely not to MDAL API, if you want to can add it to mdal_utils , but it is not necessary

@JanCaha
Copy link
Contributor Author

JanCaha commented Nov 22, 2023

Fix the issues, you mentioned @PeterPetrik . Also realized that there are few more validations that can be done on the file, so I added them + tests.

@JanCaha
Copy link
Contributor Author

JanCaha commented Nov 22, 2023

There was an issue where the new driver was trying to load another format, due to too simple canReadMesh(). Now better checks are performed to ensure that the Mike21 file can be read prior to reading. Which avoids the issue.

@PeterPetrik
Copy link
Contributor

class MeshMike21: public MemoryMesh
{
public:
MeshMike21( size_t faceVerticesMaximumCount,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary to use this new derived class ? I don't see in the code where the methods of this class are used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just copies the approach used in 2DM driver and it should allow the use of the mesh structure later. No direct use for those fuctions right now. But for 2DM driver it is rather similar, those functions are only used in ASCII_DAT format.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is fine... easier to understand it is in-Memory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

mdal/frmts/mdal_mike21.hpp Outdated Show resolved Hide resolved
mdal/frmts/mdal_mike21.hpp Outdated Show resolved Hide resolved
mdal/frmts/mdal_mike21.hpp Outdated Show resolved Hide resolved
Co-authored-by: Vincent Cloarec <[email protected]>
@PeterPetrik PeterPetrik merged commit d137b6e into lutraconsulting:master Nov 24, 2023
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants