Skip to content
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

Fix reflectivity parameters parsing in loadOBJFile #1599

Merged
merged 1 commit into from
Aug 19, 2016

Conversation

SergioRAgostinho
Copy link
Member

@SergioRAgostinho SergioRAgostinho commented Apr 27, 2016

There was a bug, attempting to make a reassignment to a reference, on the reflectivity parameters parsing stage.
I also added support to the "Tr" tag, used by meshlab, which apparently is simply the complement of the "d" dissolve parameter.

Theres was a bug, attempting to make a reasignment to a reference, on the reflectivity parameters parsing stage.
I also added support to the Tr tag, used by meshlab, which apparently is simply the complement of the "d" dissolve parameter.
@stefanbuettner
Copy link
Contributor

Hey Sérgio,

was this reassignment error a compilation error? 'Cause this rgb value is not used anywhere, is it?

Don't know anything about this Tr tag. Is there any documentation in Meshlab, which mtl-format it supports? Would be nice to have a link to the mtl-format specification PCL supports (or is working towards supporting) in the MTL reader documentation, wouldn't it.
Wikipedia links to this one, for example: http://paulbourke.net/dataformats/mtl/.

Cheers,
Stefan

@SergioRAgostinho
Copy link
Member Author

was this reassignment error a compilation error? 'Cause this rgb value is not used anywhere, is it?

It was not a compilation error. Just the underlying logic was messed up. The rgb value is used here and here. I remember I bumped on this bug while working with a specific file, and noticed the material parameters were messed up.

Don't know anything about this Tr tag. Is there any documentation in Meshlab, which mtl-format it supports? Would be nice to have a link to the mtl-format specification PCL supports (or is working towards supporting) in the MTL reader documentation, wouldn't it.
Wikipedia links to this one, for example: http://paulbourke.net/dataformats/mtl/.

True. For the Tr tag I took the description from wikipedia, but there's no reference to an actual standard.

@stefanbuettner
Copy link
Contributor

True, but don't these functions just fill that local rgb variable based on the contents of st? And afterwards rgbis not used anywhere else.

@SergioRAgostinho
Copy link
Member Author

You're right. Let me have a look tonight to see if I find my offending case :)

@SergioRAgostinho
Copy link
Member Author

Just figured out what we were overlooking in our discussion. So notice that in my fix, rgb is actually a pointer and depending on the material tag, it gets assigned a different address, e.g. to tex_Ka, tex_Kd or tex_Ks. Then in fillRGBfromXYZ and fillRGBfromRGB, rgb is passed by reference, so I'm actually changing tex_Ka, tex_Kd or tex_Ks, respectively.

@SergioRAgostinho SergioRAgostinho added the needs: code review Specify why not closed/merged yet label Aug 18, 2016
@SergioRAgostinho SergioRAgostinho added this to the pcl-1.8.1 milestone Aug 18, 2016
@jspricke
Copy link
Member

I will add some comments in line. As a side note, please put different fixes in separate commits, makes them easier to review ;).

@jspricke jspricke merged commit 048a3c8 into PointCloudLibrary:master Aug 19, 2016
@jspricke
Copy link
Member

Must have been the weather conditions, all good. Thanks!

@taketwo taketwo removed the needs: code review Specify why not closed/merged yet label Aug 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants