Add mesh support - Githubissues

MoritzWM commented 1 year ago

Hello, this is not a finished pull request, I just wanted to gather some feedback on the mesh and general data parsers. I developed napari-imodmodel alongside it and the meshes are displayed correctly.

I added a meshes list to Object. Each Mesh is a list of vertices, indices and extra data. The vertices are already reshaped to (-1, 3) during parsing, the indices are reshaped on-demand because the original list is required to calculate the indices for the extra data. I find this a bit inconsistent and wanted to ask if you have a better idea.

Only one type of mesh is supported right now (the one with control int -25), I don't have a mesh at hand where they are stored differently.

I didn't yet add any tests.

Please let me know what you think and how you would like to have it implemented.

alisterburt commented 1 year ago

silly question - I output a simple dataframe from the read function in the case of contours, it feels like it won't be possible to maintain this once mesh support is added

potential routes:

more complicated files should go via the parser and extract what they want for the object, leave the imodmodel.read() API for simple model files containing points in multiple contours?
add different return types for meshes in the read API

I'm not sure what's best here, have you thought about this?

MoritzWM commented 1 year ago

silly question - I output a simple dataframe from the read function in the case of contours, it feels like it won't be possible to maintain this once mesh support is added

potential routes:
* more complicated files should go via the parser and extract what they want for the object, leave the `imodmodel.read()` API for simple model files containing points in multiple contours?

* add different return types for meshes in the `read` API
I'm not sure what's best here, have you thought about this?

Not really to be honest. I personally think it would be easiest to return an ImodModel object. If I remember right it's how it's also done for in the mrcfile library. This object could also be used for writing models in the future (if that should ever be implemented).

alisterburt commented 1 year ago

Sounds good - to maintain compatibility with the previous API then this should be accessed via the ImodModel object with a .from_file(path: PathLike) class method (might already be the case, haven’t looked :))Sent from mobile - apologies for brevityOn 9 Aug 2023, at 19:06, Moritz Wachsmuth-Melm @.***> wrote:

silly question - I output a simple dataframe from the read function in the case of contours, it feels like it won't be possible to maintain this once mesh support is added potential routes:

more complicated files should go via the parser and extract what they want for the object, leave the imodmodel.read() API for simple model files containing points in multiple contours?
add different return types for meshes in the read API

I'm not sure what's best here, have you thought about this?

Not really to be honest. I personally think it would be easiest to return an ImodModel object. If I remember right it's how it's also done for in the mrcfile library. This object could also be used for writing models in the future (if that should ever be implemented).

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>

MoritzWM commented 1 year ago

Hi, sorry for taking so long. I've added an example to the readme. The from_file function was already there, so I just added ImodModel to all.

In the future, one could add a nicer API for contour extra values, but I guess that's stuff for a future pull request.

Let me know if there's anything you're still missing in the PR.

jojoelfe commented 9 months ago

Hi,

I've been thinking about importing imod meshes into blender so this PR would really help with that. I am just checking what the status is and whether I can maybe help?

Best,

Johannes

alisterburt commented 9 months ago

Hey @jojoelfe and @MoritzWM ! @MoritzWM sorry for the huge delay here, this came in whilst I was moving to the US and got lost in the fold. @jojoelfe I've added you as a maintainer to the project - please feel free to merge/work on this PR with @MoritzWM and you can cut a release whenever you feel ready! pushing to main with the tag "v" where <ver> is the semantic version should trigger the test/release process on github actions :-)

If anything isn't clear/any questions pop up I'm here and happy to help!

jojoelfe commented 9 months ago

Thanks @alisterburt ! @MoritzWM Are you still interested in joining this? I haven't looked into it in great detail, going over the comments briefly I only wonder if this still should be using pydantic 1 or make the switch to 2?

alisterburt commented 9 months ago

@jojoelfe I'm keen for pydantic 2, I assume the ecosystem won't take too long to fully move over and I'd rather be ahead than behind. If compatiblity can be maintained for pydantic 1 that would be great but no hard requirement :-)

MoritzWM commented 9 months ago

Hey, @alisterburt no problem, there was no rush ;)

@jojoelfe I'm very keen on getting this merged. Have you tested it already in blender? If it works fine than I see no reason not to merge. A possible improvement would be to remove unused vertices and indices as done by ChimeraX, but this could also be a separate PR.

Regarding pydantic 1 and 2: as I understand from this PR napari is compatible with both thanks to a compatibility layer. We could do the same. This could be a separate PR or we do it already here, I don't mind.

alisterburt commented 9 months ago

@MoritzWM I've also made you a maintainer here! Please still open PRs with changes and try to keep changes minimal to simplify review - review should still be requested but this should stop you from getting blocked if I or @jojoelfe is unavailable and you need to move forwards :-)

jojoelfe commented 9 months ago

I haven't tested the import into blender yet, but opening some of my files works without issue. To maintain compatibility with both pydantic v1 and v2 we can just:

from pydantic.version import VERSION as PYDANTIC_VERSION

and then make the config class conditional:

class GeneralStorage(BaseModel):
    """https://bio3d.colorado.edu/imod/doc/binspec.html"""
    type: int
    flags: int
    index: Union[float, int, Tuple[int, int], Tuple[int, int, int, int]]
    value: Union[float, int, Tuple[int, int], Tuple[int, int, int, int]]

    if PYDANTIC_VERSION < '2.0':
        class Config:
            smart_union = True

smart_union is only deprecated in v2 because it is the default behavior.

This seems a small price to pay for making sure this works with most packages. We should make a hatch test matrix with both pydantic versions, but that can be a separate PR.

jojoelfe commented 9 months ago

I tested this with blender and it works great. I pushed the changes I described above and enabled tests against both pydantic versions.

@alisterburt @MoritzWM as long as there are no vetos, I'll go ahead and merge

alisterburt commented 9 months ago

Amazing work both, thanks you!!

alisterburt commented 9 months ago

I pushed v0.0.9 which includes this, should be up on pypi once the test/deploy matrix finishes https://github.com/teamtomo/imodmodel/actions/runs/7500839757

Thanks both! Would be great if we could follow up with documenting this :-)

teamtomo / imodmodel

Add mesh support #7