New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Groups not separated as different shapes #138

Closed
sunweilun opened this Issue Sep 11, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@sunweilun

sunweilun commented Sep 11, 2017

Dear 'tinyobjloader' authors,

Thanks for your contribution. 'tinyobjloader' is a very useful tool for me. Recently, I had some issues with 'tinyobjloader' ignoring the 'g' tag. I hope we can fix it to further improve 'tinyobjloader'. Below is what I have found:

When 'g' immediately follows 'usemtl' in the obj file, the loader ignores the 'g' tag and does not create a new shape.

Related lines (1621-1626) in tiny_obj_loader.h:

  // flush previous face group.
  bool ret = exportFaceGroupToShape(&shape, faceGroup, tags, material, name,
                                    triangulate);
  if (ret) {
    shapes->push_back(shape);
  }

Here, it is possible that all faces in the buffer have been dumped because of the 'usemtl' tag above the 'g' tag at this point. This way, 'exportFaceGroupToShape' will return 0 and a new shape for the 'g' tag will not be created. What I think we really want is that we create a new shape as long as the previous shape is not empty.

Suggested change:

  // flush previous face group.
  exportFaceGroupToShape(&shape, faceGroup, tags, material, name,
                                    triangulate);
  
  if (shape.mesh.indices.size()) {
    shapes->push_back(shape);
  }

Let me know whether this makes sense.

Thanks!

@syoyo

This comment has been minimized.

Owner

syoyo commented Sep 11, 2017

Thank you for finding an issue.

Suggested change:

This looks nice.

Do you have small test .obj/.mtl to reproduce this issue?
Before fixing the issue, I would like to add test dataset as a regression.

@sunweilun

This comment has been minimized.

sunweilun commented Sep 11, 2017

@syoyo

This comment has been minimized.

Owner

syoyo commented Sep 11, 2017

Attached. It is a cube with 2 groups

Hmm... It looks file attachments are missing in your comment.

@sunweilun

This comment has been minimized.

sunweilun commented Sep 11, 2017

@syoyo

This comment has been minimized.

Owner

syoyo commented Sep 11, 2017

Did it work this time?

Unfortunately no.

Did you attach files through github web UI?

@sunweilun

This comment has been minimized.

sunweilun commented Sep 11, 2017

test.zip

This time? Sorry, I was using emails.

@syoyo

This comment has been minimized.

Owner

syoyo commented Sep 11, 2017

Thanks!

I did quick fix and pushed as group-fix branch https://github.com/syoyo/tinyobjloader/tree/group-fix

But I'm not sure this change is safe.
IIRC, the may may affects with previous issue: #104

Thus after some tests and code reviews, the change will be merged into master

@sunweilun

This comment has been minimized.

sunweilun commented Sep 11, 2017

@syoyo syoyo closed this in 1065d7c Sep 14, 2017

@syoyo

This comment has been minimized.

Owner

syoyo commented Sep 14, 2017

It looks it does not produce any side effects, thus merged into master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment