UseMtl needs to trigger new group

[h1cks]

Found on some exports from Blender that the Object format isn't completely parsed correctly.

Where a USEMTL keyword appears in the facelist, it sets the texture in the current group. But, because that material applies to all the faces it parses after, not the current group. As such, all faces have the material applied of the last USEMTL encountered. To fix this.

I updated the class to push a new group.

public class UseMaterialParser : TypeParserBase, IUseMaterialParser
{
    private readonly IElementGroup _elementGroup;
    private readonly IGroupDataStore _groupDataStore;


    public UseMaterialParser(IElementGroup elementGroup, IGroupDataStore groupDataStore)
    {
        _elementGroup = elementGroup;
        _groupDataStore = groupDataStore;
    }

    protected override string Keyword
    {
        get { return "usemtl"; }
    }

    public override void Parse(string line)
    {
        _groupDataStore.PushGroup(line);  // create a new group 

        _elementGroup.SetMaterial(line); 
    }

I also added to the datastore class some changes

    public void PushGroup(string groupName)
    {
        if (_currentGroup != null)
        {
            if (_currentGroup.Faces.Count == 0)  // if there are no faces, we dont need to create a new group/
            {
                return;
            }
        }

        _currentGroup = new Group(groupName);            
        _groups.Add(_currentGroup);
    }

    private void PushGroupIfNeeded()
    {
        if (_currentGroup == null)
        {
           PushGroup("default");
        }
    }

Just highlighting that the parser currently doesn't create enough groups.

Also, highlight that the exports where it uses the keyword "o" instead of "g" should also create a new group.

[h1cks]

Just another thing, It might be better to convert part of this to a recursive descent parser. Great start, but for the more advanced features of the .OBJ file. I will need additional changes.

[h1cks]

@chrisjansson - FYI

[chrisjansson]

Hehe yes, it's hardly an attempt at a feature complete parser to say the least.

I'll gladly accept a pull request with the changes preferably with a test case with a short example input .obj along the lines of https://github.com/ChrisJansson/ObjLoader/blob/master/source/CjClutter.ObjLoader.Test/Loaders/ObjLoaderTests.cs

Also, can you elaborate on the benefits you can see from refactoring the code in the style of a recursive descent parser? It's been a few years since I looked at this code the last time.

[h1cks]

HI, was just adding some stuff to uplift it :)

I will dig up the file that I used to test the edge case. But effectively, the geometry has "usemtl" tags through out the face list (which according to the file format is legal). I just added a bit more complexity to allow myself to load the file :)

The recursive descent only really gets benefits if you have multiple objects and you want to break them into different return groups. You do need to preface the recursive descent by tokenising the whole file which makes your life a bit easier when parsing, but this is all above and beyond what you would do to load an .obj file in. It gives you benefits only if you want to build more complex structures from your export file. For simple objects and texturing, its fine. But if you want to build a hierarchy then recursive descent allows you to maintain your spot in the hierarchy through a pop/push stack and also traverse or backtrack if need be.

It's a bit of overkill for loading, I've done 2 such pieces before and they were pulling apart mathematical formulas and precedence.

I will try and add the file I loaded (which was a airplane), just a bit flat out lately so don't know when I will do the test code.