Refactor and writer

[elrnv]

This PR is unfortunately very large, and makes a bunch of improvements (IMO) as well as adding a writer for .obj as well as .mtl files.

I am open to making changes or reintroducing some of the code/APIs that have been removed here if need be.

Below is a summary of noteworthy changes.

• Split Obj into a path and ObjData, which can be compared with PartialEq, since two objs from different files can be equivalent.
• Updated Mtl to be a lazily loaded vector of actual material data. It is explicitly stored in the ObjData struct, and referenced by each group using Arcs instead of Cow.
• ObjMaterial enum wraps the Material struct. ObjMaterial is either a reference to a material (a name, before the materials are loaded) or an Arc pointing to the Material data stored in one of the material libs.
• Refactored the reader API to accept Read types instead of BufRead types, which makes the API easier to use (the user doesn't have to create BufReader themselves).
• The string "default" is saved as a constant and referenced by variable names DEFAULT_OBJECT and DEFAULT_GROUP.
• Added a WriteToBuf trait that defines the interface for structs to write their data to a buffer. This is similar to the Display trait but is specific to the Obj type. It is exposed publicly since polygons in Obj are generic.
• Added more documentation to structs and fields.
• Added a writer for the obj and mtl file formats.
• Added a round trip test with and without materials for the sponza test file.

Edit: Additional changes followed from the discussion:

• In an effort to simplify the API we removed the generic parameter for polygons throughout, replacing it with a converter from SimplePolygon to genmesh::Polygon.
• Made load and save functions accept impl AsRef<Path> instead of just &Path making them easier to use.
• Filtered the crate through clippy and rustfmt. Also added CI jobs to run rustfmt and clippy.

[elrnv]

Fixes #5

[kvark]

Oh wow, grand work here! I'll try to review later today.

[elrnv]

Thank you :) I had a lingering question about having all the Obj data be generic over polygons. Is this strictly necessary? My impression was that the data format in these import/export formats should just be optimal for reading and writing since there is no way to predict how the user will want their polygons represented anyways (could be in structure of arrays format for instance).

The reason I ask is I think it would be nicer to have Obj have no generic parameter to make it easier to use. For example loading a file, I think should look like:

let obj = Obj::load(&Path::new("file.obj"))?;

but we currently need

let obj = Obj::<SimplePolygon>::load(&Path::new("file.obj"))?;

or with this PR

let obj: Obj = Obj::load(&Path::new("file.obj"))?;

since SimplePolygon was made default. If the idea was to just switch between SimplePolygon and use genmesh, we could still do it without the generic parameter, but just with conditional compilation...

What do you think?

PS: I could also add an impl AsRef<Path> for the load and save arguments to make it even less verbose to use those functions.

[kvark]

If the idea was to just switch between SimplePolygon and use genmesh, we could still do it without the generic parameter, but just with conditional compilation...

I don't think this will work well. Features are supposed to be additive, and if I understand your suggestion, it will change the signature of functions based on the genmesh feature...

[elrnv]

This looks just wonderful, although I haven't tried to carefully inspect all the logic. I assume it works for some cases, and I'm happy to see the tests!

Thanks I wasn't expecting this to be reviewed so soon since there are quite a few changes. I think the most questionable logic is in the WriteToBuf impl for Group, which reads

        // When index is greater than 0, we know that this group is the same as the previous group,
        // so don't bother declaring a new one.
        if self.index == 0 {
            writeln!(out, "g {}", self.name)?;
        }

So I assume here that the increased index will always indicate that this group should be merged with the previous when writing.

The biggest thing I would propose is, since you are basically rewriting the whole thing, that we refresh the formatting. We'd want to do the stable rustfmt, so we'll need to clear the config of all the things that are nightly-only. All of that, of course, needs to be in a separate commit.

Would you want to do this? Or leave for somebody to follow up?

Thanks! I would be happy to do that. Should we add fmt and clippy to CI as well? I can simply run rustfmt for this PR and add another PR for the CI additions.

[kvark]

It would be welcome to add clippy, we can do it here or as a follow up. It's not implying as big of a change as formatting :)

So I assume here that the increased index will always indicate that this group should be merged with the previous when writing.

The knowledge of the OBJ details is completely out of my head right now, so I can't validate this without doing a small research. Do you think we could turn that assumption into an assertion?

[elrnv]

If the idea was to just switch between SimplePolygon and use genmesh, we could still do it without the generic parameter, but just with conditional compilation...

I don't think this will work well. Features are supposed to be additive, and if I understand your suggestion, it will change the signature of functions based on the genmesh feature...

I see that makes sense. What is the benefit of using genmesh Polygons here instead of just SimplePolygon? Could we simply have one or the other?

[elrnv]

FYI, beyond the things we discussed, I sneaked in the impl AsRef<Path> in the load/save fns I was talking about earlier.

[kvark]

What is the benefit of using genmesh Polygons here instead of just SimplePolygon? Could we simply have one or the other?

You could use genmesh-provided utilities, like LruIndexer, without an extra conversion step. I was doing this in https://github.com/three-rs/three/ and quite enjoyed the integration.

[elrnv]

You could use genmesh-provided utilities, like LruIndexer, without an extra conversion step. I was doing this in https://github.com/three-rs/three/ and quite enjoyed the integration.

I see that makes sense, thanks for the example! Looking briefly at that code, if we were to simply provide a converter from SimplePolygon to Polygon<IndexTuple>, for instance with:

#[cfg(feature = "genmesh")]
impl SimplePolygon  {
    fn into_genmesh_poly(self) -> Polygon<IndexTuple> {
        std::convert::TryFrom::try_from(self).unwrap()
    }
}

#[cfg(feature = "genmesh")]
impl std::convert::TryFrom<SimplePolygon> for Polygon<IndexTuple> {
    type Error = ObjError;
    fn try_from(gs: SimplePolygon) -> Result<Polygon<IndexTuple>, ObjError> {
        match gs.0.len() {
            3 => Ok(Polygon::PolyTri(Triangle::new(gs.0[0], gs.0[1], gs.0[2]))),
            4 => Ok(Polygon::PolyQuad(Quad::new(gs.0[0], gs.0[1], gs.0[2], gs.0[3]))),
            n => {
                return Err(ObjError::GenMeshTooManyVertsInPolygon {
                    vert_count: n,
                })
            }
        }
    }
}

then the only real code change this would require in the three-rs example is the addition of

.map(|p| Polygon::try_from(p).unwrap())

when iterating over the polys.

The other alternative is to make SimplePolygon a non-exhaustive enum. This will at least push the concern down to where the user needs to deal with polygons.

I made a prototype based on this PR for the first variation above in https://github.com/elrnv/obj/tree/non-generic-obj. Let me know what you think.

[kvark]

Ok, I think this is convincing enough, thank you! We can proceed with your plan then :)

[elrnv]

Looks like it's good to go, thanks for all the feedback!

Edit: I have the patch for three-rs ready here: https://github.com/elrnv/three/tree/update-obj

[kvark]

Alright, time to land this!