Obj format output in reconstruction, and normals outputting for obj format

[rezural]

This adds obj files as a supported format from reconstruction, and enables normals output if requested.

Still need to add docs updates, and I have not tested this thoroughly, so apologies if there are mistakes.

Thanks

[w1th0utnam3]

Thanks!

I think I would prefer to put the match on the file extension somewhere else, similar to how the read_particle_positions is in the io module. Maybe simply a write_surface_mesh function with a similar structure in io.rs. Feel free to do something about this, otherwise I'll check it out over the weekend.

[rezural]

OK, I'll hopefully have a look at this tomorrow.

I'd been coding for quite a bit by the time I needed to add this in, so didnt give too much thought the the architecture :D

[rezural]

Small commit added to output normals indices on the face elements

[rezural]

If you have some time, I think this is the code structure you are looking for, but there is a problem with what write_vtk expects to be sent.... there may be some hackery to convert the MeshWithData into an Into<DataSet>, but I'm struggling with it.

This latest commit doesnt compile, the error is here:

error[E0277]: the trait bound `&MeshWithData<R, M>: std::convert::Into<splashsurf_lib::vtkio::model::DataSet>` is not satisfied
   --> splashsurf/src/io.rs:191:17
    |
191 |                 vtk_format::write_vtk(mesh, &output_file, "mesh").with_context(|| {
    |                 ^^^^^^^^^^^^^^^^^^^^^ the trait `std::convert::Into<splashsurf_lib::vtkio::model::DataSet>` is not implemented for `&MeshWithData<R, M>`
    | 
   ::: splashsurf/src/io/vtk_format.rs:43:8
    |
43  | pub fn write_vtk<P: AsRef<Path>>(
    |        --------- required by a bound in this
44  |     data: impl Into<DataSet>,
    |                ------------- required by this bound in `vtk_format::write_vtk`
    |
help: consider introducing a `where` bound, but there might be an alternative better way to express this requirement
    |
173 | ) -> Result<(), anyhow::Error> where &MeshWithData<R, M>: std::convert::Into<splashsurf_lib::vtkio::model::DataSet> {
    |                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: could not compile `splashsurf`

Thanks

[w1th0utnam3]

Actually &MeshWithData does implement Into<DataSet> but you have to ensure that its MeshT also implements Into<DataSet> as you can see here: https://github.com/w1th0utnam3/splashsurf/blob/a5e5497e08d32bce92d5f5016ae8a33550f5598d/splashsurf_lib/src/mesh.rs#L614-L623

If you copy these trait bounds to the write_mesh implementation it compiles. However we can write it a bit less cumbersome as:

pub fn write_mesh<'a, R: Real, MeshT: Mesh3d<R>, P: AsRef<Path>>(
    mesh: &'a MeshWithData<R, MeshT>,
    output_file: P,
    _format_params: &OutputFormatParameters,
) -> Result<(), anyhow::Error>
where
    &'a MeshWithData<R, MeshT>: Into<DataSet>

Did you check that the obj files with normals are correct and can be read by other tools?

[rezural]

I have tested the normals (my code was originally broken, but it is fixed now, & I will double check)

[rezural]

Thanks for the help. I have made those changes, they compiled fine.

I have tested the output, but I shall do so again with an example from splashsurf (I was symlinking to the splashsurf binary, not 100% sure i was using the right binary).

[rezural]

I am getting some corrupted files on large data sets, with many files. I think this is unrelated to this change (I believe the files were rsynced from the server before the files were fully written). I'll get to the bottom of it and let yout know.

Would you accept some integration tests with obj-rs?

[rezural]

OK, no more corrupted files, it was an rsync problem.

I'll look into adding some tests to make sure the output is good.

[w1th0utnam3]

Sounds great! Not sure what your plan is, but maybe you can manually construct a simple mesh in the test for reference. This way we don't have to rely on a reconstruction giving identical results.

[w1th0utnam3]

Ah, missed your comment with obj-rs. Yeah, you can add it as a dev dependency. Do you want to make a round trip test, like storing with splashsurf, loading with obj-rs and then comparing?

[rezural]

OK, I'll have a look at it. I was thinking to go through obj-rs, but perhaps just some simple tests running through the convert command to start with.

[rezural]

OK, adding the tests is harder than I initially thought (unless I put it somewhere stupid).

Makes sense to either:
move the splashsurf/src/io.rs into splashsurf_lib/src, so we can get access to the io stuff.
or add a dependency from splashsurf_lib to splashsurf, so we can get access to io....

Which of these would you prefer?

I also have a small update #30, to make it handle more formats a bit easier. If that looks ok to you, I will rebase off master when you merge it.

[rezural]

Or perhaps merge this, and the io restructure, and I will add in the tests in a new PR.

Cheers

[w1th0utnam3]

Yes, the current situation with the IO is a bit annoying for me as well for some tests in splashsurf_lib (because this results in some code duplication for the tests). I definitely don't want to have a dependency on the binary crate splashsurf in splashsurf_lib (the former should more or less only provide the CLI).

It would be possible to move the IO stuff to splashsurf_lib although this feels like a lot of clutter if you just want to run a surface reconstruction in memory/on the fly in some other crate. Of course one could move this behind a feature flag, like the current vtkio trait implementations. Alternatively I could create a separate splashurf_io crate. Do you have an opinion on this?

In any case this should be part of a different PR or I can take a look at this myself after deciding how to refactor this. However, for the current situation and this specific test couldn't you just put it somewhere in splashsurf?

[rezural]

Sounds like a good idea. I'll put it in splashsurf for now.

[rezural]

Hrm, this presents issues as well.

It's not trivial ATM to pull in from an obj or ply file (it would be just as easy to add it to the convert command...
I'm not sure how to create a dead simple .vtk file (if you have a simple cube with normals laying around... that would help)

I may perhaps just create the mesh entities directly, but that is not exactly trivial either.

Gotta drop this today. If you have a better idea for this, let me know.

Cheers

[rezural]

I've added support for importing meshes from ply files, here: #31.

This will make it possible to test outputting obj file from a simple ply file.

I'll add tests once you've merged that PR (hopefully :D)

[w1th0utnam3]

I'll merge this for now, we can add tests a bit later.