Update Mtl to include more options.

[yperess]

I've added all the following options to the Mtl object as well as more detailed map values replacing the old string value with an object that holds the various options. The information used for this and the default values was taken from https://en.wikipedia.org/wiki/Wavefront_.obj_file

• illum
• map_Ka
• map_Ks
• map_Ns
• map_d
• bump
• disp
• decal

[javagl]

Thanks for these contributions.

That's a lot of changes, and I'll have to allocate far more time to review and test this. From quickly skimming over it, I have to admit that I'm veeery hesitant to pull some of these. Based on a first look, a few comments, hoping that they do not come across too reluctant or harsh or so:

• I know that an MTL can contain far more than the options that are currently supported by the Mtl class. However, I have never encountered such an MTL. Do they actually appear in practice? In general, adding support for more options is fine, but I'll have to review the specs to at least roughly understand what they mean.

BTW: The OBJ file format itself also supports far more options. This "smoothing group" stuff and such - you can even have Bezier patches in OBJ! But the same here: I have never seen it in reality. Basically all OBJ files that I ever saw are just "indexed face sets", as a smallest common denominator for pure geometry data interchange.

• Adding dependencies to additional libraries just for Nullable and NotNull is, in my opinion, just not worth it. Right now, the OBJ JAR is a standalone JAR that people can download and drop into their IDE (and that's what the Google/AR people did). It is "minimal" in the sense that it just offers exactly what 99% of all users need, in the simplest form that I could imagine, namely some functions that can load 99% of all real-world OBJs and provide the data so that it can be rendered it with OpenGL.

• Adding the Rect3D class seems reasonable at the first glance. In fact, I had a BoundingBox class in the ObjSplitting class at https://github.com/javagl/JglTF/blob/master/jgltf-obj/src/main/java/de/javagl/jgltf/obj/ObjSplitting.java#L262 , which (in the meantime) has been moved to the OBJ library ... where the BoundingBox became superfluous. And that's the point that I'm hesitant about: Most people simply will not need the bounding box. The commit comment said that it was "cheaper" to compute it on the fly, but even cheaper is... to not compute it at all. So I'm pretty sure that loading something like the "Happy Buddha OBJ" model with 500000 vertices will be slower when the bounding box is computed. Regardless of performance, I'm not sure about the class structure, but I'll have to read it more thoroughly

• Some details. I know, I'm a dick about that: No m prefix for fields, public classes should be final unless designed for inheritance, JavaDocs for everything (yes, everything) ... However, this is stuff that I would not expect in a PR, but in doubt do on my own after pulling...

Again, this sounds very reluctant, but I'm just trying to be considerate, particularly regarding the dependencies and interfaces. However, extending the MTL support is fine, and I'll probably at least pick some of the related commits after reviewing.

[yperess]

No worries at all, it's your library and you're allowed to want things a certain way. I'll update to match your style. By no means should you feel you have to accept this PR, I can very much see the merit in keeping things simple and catering to the common case. My team can keep working off our fork and develop that to be a more robust loader which we need for our project. I have a sample rendering library working now and will post it later tonight. I'll include a link to it when I finish.

[javagl]

Great, it would be good to see the intended application case for the extended MTL. I'm really curious: Do you really have MTL files that contain this detailed information? (In doubt, I'd check what e.g. blender can actually export into an MTL. They do have this detailed information, but wonder whether they are writing it out as well...)

[yperess]

I have some of the fields, but as a loader I figured load everything and it's the job of the renderer to pick and choose what to support. That's 100% my own personal take on it and like I said, if you want to keep this library lean I completely understand. Working on some polishing work now and will post shortly.

[yperess]

Please be kind :) this was all written very quickly and is in a very early stage. https://github.com/GoMeta/AndroidObjRender-Demo

We've added a few samples and will continue to add more as we find new ones that stress the system. The current ones I'm working on fixing are the fireplace, dump truck, and the skeleton/robot thing.

[javagl]

Thanks for the pointer. I never used Kotlin, but it looks rather readable at the first glance. I'll drop you a note here when I've had a closer look at the Mtl loader updates etc.

[javagl]

Just a short note: I've started integrating the extended MTL support (nothing committed yet, just locally). There may be minor structural differences to your PR, but these would only be details.

EDIT: A somewhat more fiddly issue may be how to properly write the resulting MTL files. Considering the "default values" in the TextureOptions, one would have to detect more precisely which of the options deviate from their default values, and only then they should be written.

Just noticed that this is already largely covered in https://github.com/javagl/Obj/pull/7/commits/3a0bdc3d8f95415b2ef5a5b26a4579cad50f107f#diff-cb1354711666061ecfd54be042277374

(You may have seen the current TestMtlWriter, which is very pragmatic (and very strict) : It just reads an Mtl and then writes it, and expects the result to be the same as the input file...).

[javagl]

BTW: Do you think that the benefit of adding semantics to the enumeration values like the ImfChannel and IlluminationMode is really worth the effort?

For example users who want to interpret something like the IlluminationMode will basically have to do this:

IlluminationMode mode = mtl.getIlluminationMode();
switch (mode) {
    case COLOR_ON_AMBIENT_OFF: return new TrivialRenderer();
    case COLOR_ON_AMBIENT_ON: return new SimpleRenderer();
    ...
    SHADOW_ON_INVISIBLE_SURFACES: return new WhatTheHeckDoesThisRendererDo();
}

I'm not sure whether this is so much more convenient than

int mode = mtl.getIlluminationMode();
switch (mode) {
    case 0: return new TrivialRenderer();
    case 1: return new SimpleRenderer();
    ...
    10: return new WhatTheHeckDoesThisRendererDo();
}

Of course, the semantics may be helpful, but considering that the main goal of the library (particularly regarding MTL) is to serve as a parsing layer for the contents of an MTL file, people will anyhow have to translate the contents of an Mtl or TextureOptions objects into settings for their renderer.

Again, this may be a misconception: I hadn't seen "complex" MTL files before. In the samples of your app, I saw that some of them specify the illum and the Ni (which seems to still be missing, BTW), Ke (which is likely a (non-standard!) "emissive" value), but haven't yet looked into the details. It seems to not be used in https://github.com/GoMeta/AndroidObjRender/blob/master/src/main/java/io/gometa/support/obj/mlt/MtlShaderImpl.kt , at least...

[yperess]

You know after I'd already done it I was thinking I should have just defined them as static finals ints in a namespace like ImfChannel and IlluminationMode this way people can just use 1 or the name for verbosity.

[javagl]

I have added a preliminary implementation of the extended MTL support in a new branch at https://github.com/javagl/Obj/commit/5bd204cb8d1a434897c7b3b786eb801cabff5832

Some differences between your first commit and this one are rather related to some "style", which I aligned to be in line with the existing classes. Although, admittedly, some of this does not make terribly much sense: Mtl and TextureOptions are rather function-free "data containers", but Mtl had been an interface, and I didn't intend to change that. This is also not relevant for the user of the library.

What's more relevant for the user: The methods for accessing the properties are named according to the properties in the file. Although a name like getOriginOffset() is far more sensible than getO(), I did this to be consistent with the original methods in the Mtl class and the MTL specification. This may no be the best choice in terms of API design, and it feels a bit awkward, but whatever names I could come up with, they would likely create room for confusion: I would call it "opacity" or (its inverse) "transparency", but never "dissolved", as they did with the d parameter in MTL. At this point, I would have to write "my own" MTL documentation, which is something I'll certainly not do.

However, other, conceptual differences are:

• The IlluminationMode, ImfChannel and Type are just encoded as Integer and String. Adding an enum seems a bit like overkill, but your suggestion to add the "known" values as constants sounds reasonable - I could add this later, if desired
• The properties in the Mtl and TextureOptions are by default null when no information was read from the input file. I thought about a sensible way to offer the default values here, but I think that, the information about whether something had been contained in the input file has to be encoded somehow.
• Some properties had been missing, e.g.
  • Ni, the index of refraction
  • cc, the color correction flag
  • -halo, a flag that may be added to d, a reflection parameter
  • Tf, the transmission filter color
  • refl, the reflection maps. And there may obviously be multiple of these!

These changes are largely backward compatible, in the sense that they only add information to the Mtl interface. I'm still a bit hesitant to merge this into master, because it's a lot of information that is added, and when it is added, I'll never be able to take it back. But if people who are really using this detailed MTL information think that it makes sense, it could be added in the next release.

[yperess]

This all sounds reasonable to me, I'll take a closer look tonight.

On Sun, Feb 18, 2018, 11:10 Marco Hutter notifications@github.com wrote:

I have added a preliminary implementation of the extended MTL support in a new branch at 5bd204c https://github.com/javagl/Obj/commit/5bd204cb8d1a434897c7b3b786eb801cabff5832

Some differences between your first commit and this one are rather related to some "style", which I aligned to be in line with the existing classes. Although, admittedly, some of this does not make terribly much sense: Mtl and TextureOptions are rather function-free "data containers", but Mtl had been an interface, and I didn't intend to change that. This is also not relevant for the user of the library.

What's more relevant for the user: The methods for accessing the properties are named according to the properties in the file. Although a name like getOriginOffset() is far more sensible than getO(), I did this to be consistent with the original methods in the Mtl class and the MTL specification. This may no be the best choice in terms of API design, and it feels a bit awkward, but whatever names I could come up with, they would likely create room for confusion: I would call it "opacity" or (its inverse) "transparency", but never "dissolved", as they did with the d parameter in MTL. At this point, I would have to write "my own" MTL documentation, which is something I'll certainly not do.

However, other, conceptual differences are:

• The IlluminationMode, ImfChannel and Type are just encoded as Integer and String. Adding an enum seems a bit like overkill, but your suggestion to add the "known" values as constants sounds reasonable - I could add this later, if desired
• The properties in the Mtl and TextureOptions are by default null when no information was read from the input file. I thought about a sensible way to offer the default values here, but I think that, the information about whether something had been contained in the input file has to be encoded somehow.
• Some properties had been missing, e.g.
  • Ni, the index of refraction
  • cc, the color correctio flag
  • -halo, a flag that may be added to d, a reflection parameter
  • Tf, the transmission filter color
  • refl, the reflection maps. And there may obviously be multiple of these!

These changes are largely backward compatible, in the sense that they only add information to the Mtl interface. I'm still a bit hesitant to merge this into master, because it's a lot of information that is added, and when it is added, I'll never be able to take it back. But if people who are really using this detailed MTL information think that it makes sense, it could be added in the next release.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/javagl/Obj/pull/7#issuecomment-366526620, or mute the thread https://github.com/notifications/unsubscribe-auth/AJF_-hWepvVl2P-m2SoTvyqwd3VH0AfTks5tWEtegaJpZM4SCwJf .

[javagl]

There seem to be some possible extensions for the MTL format (mentioned at https://en.wikipedia.org/wiki/Wavefront_.obj_file#Physically-based_Rendering ). Particularly, some of them seem to include PBR properties. Considering the increased attention that PBR has gained recently, it might be worthwhile to consider how such information may be retained and at least somehow extracted by the MTL reader...

(BTW: I considered to change the TextureOptions to no longer be an interface. Right now, it looks a bit "overengineered". But for the user of the library, this will not make a difference, so this is only a minor issue)

[javagl]

Yes, sorry. It got buried under other priorities, and ... there doesn't seem to be such a great demand for extended MTL support. But I still feel guilty for "ignoring" this PR and the alternative branch at https://github.com/javagl/Obj/commit/5bd204cb8d1a434897c7b3b786eb801cabff5832 until now...