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

Mesh stream read #29

Closed
rtoepfer opened this Issue Jan 17, 2017 · 10 comments

Comments

Projects
None yet
4 participants
@rtoepfer

rtoepfer commented Jan 17, 2017

https://github.com/google/draco/blob/master/io/mesh_io.h

Line 59 - shouldn't the read occur at the initial tellg position since we are using that position to determine read size?

@ondys

This comment has been minimized.

Collaborator

ondys commented Jan 18, 2017

Thanks for the report, the issue has been already fixed.

@ondys ondys closed this Jan 18, 2017

@rtoepfer

This comment has been minimized.

rtoepfer commented Jan 18, 2017

Would be nice to get credit, you didn't fix it until I posted the pull request!

@kaben

This comment has been minimized.

kaben commented Jan 18, 2017

Yes. @ondys, you should in future give credit where due. @rtoepfer's pull request, which he submitted yesterday, contains this patch for io/mesh_io.h:

   // Determine size of stream and write into a vector
    auto is_size = is.tellg();
    is.seekg(0, std::ios::end);
 -  is_size = is.tellg() - is_size;
 -  is.seekg(0, std::ios::beg);
 -  std::vector<char> data(is_size);
 -  is.read(&data[0], is_size);
 +  auto vec_size = is.tellg() - is_size;
 +  is.seekg(0, is_size);
 +  std::vector<char> data(vec_size);
 +  is.read(&data[0], vec_size);

Your diff to same, which you wrote half an hour ago, contains what appears to be a rewrite of his patch:

    // Determine size of stream and write into a vector
 -  auto is_size = is.tellg();
 +  const auto start_pos = is.tellg();
    is.seekg(0, std::ios::end);
 -  is_size = is.tellg() - is_size;
 -  is.seekg(0, std::ios::beg);
 +  const std::streampos is_size = is.tellg() - start_pos;
 +  is.seekg(start_pos);
    std::vector<char> data(is_size);
    is.read(&data[0], is_size);

Not cool.

@ondys

This comment has been minimized.

Collaborator

ondys commented Jan 18, 2017

It has been actually fixed internally shortly after the report yesterday. It just takes time to propagate the changes to the public repo. In any case, we definitely welcome any external pull requests and we will be happy to merge them as long as they don't conflict with our internal fixes.

@kaben

This comment has been minimized.

kaben commented Jan 18, 2017

To be clear, you wrote:

has been actually fixed internally shortly after the report

Did you mean to say "shortly before"? Or did the fix take place after the report, but before you had a chance to read the report?

@ondys

This comment has been minimized.

Collaborator

ondys commented Jan 18, 2017

Shortly after. We monitor the issue tracker and any reported bugs are fixed internally first and then they are pushed to the public repo (usually we do the final push only after sufficient number of changes accumulate, unless the fix is considered critical). Note that the pull request in question was posted after about one day after the actual report.

@rtoepfer

This comment has been minimized.

rtoepfer commented Jan 18, 2017

@waldyrious

This comment has been minimized.

waldyrious commented Jan 19, 2017

@ondys are you guys planning to eventually do all development in the open? Otherwise there will be such cases of duplicated work, which discourages contributions.

@ondys

This comment has been minimized.

Collaborator

ondys commented Jan 19, 2017

@waldyrious we may eventually move it to open but we don't plan to do it in the near future. To reduce the chance of duplicate work, we will do our best to write a note on issues when they are picked up by internal developers.

@waldyrious

This comment has been minimized.

waldyrious commented Jan 19, 2017

Understood -- thanks for the clarification.

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