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.

Sign up for GitHub

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

Jump to bottom

added code for iterating over point cloud #10

Merged
merged 1 commit into from
Feb 4, 2020
Merged

added code for iterating over point cloud #10

merged 1 commit into from
Feb 4, 2020

Conversation

chrisasc
Copy link
Contributor

Extended the existing code with a loop to iterate over point cloud and display every value

@chrisasc chrisasc requested a review from knatten August 12, 2019 14:43
@chrisasc chrisasc self-assigned this Aug 12, 2019
SatjaSivcev
SatjaSivcev reviewed Aug 12, 2019
View reviewed changes
Copy link
Contributor

@SatjaSivcev SatjaSivcev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have only these two 'cosmetic' fixes. @knatten can you please check the important stuff (code)? :)

@SatjaSivcev
Copy link
Contributor

I was thinking about suggesting to rename the sample to something that describes this addition but I don't have a good idea. Do you? readZDFExtractData, readZDFIterate, readZDFGetData don't sound nice to me.

Also, the sample description needs to be modified and the README.md updated.

@chrisasc
Copy link
Contributor Author

Will do these changes now.

knatten
knatten suggested changes Aug 13, 2019
View reviewed changes
Copy link
Contributor

@knatten knatten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some suggestions for improvements to the code.

As for the commit messages, check my post on Teams from last week. In particular they should be capitalized and in an imperative mood.

There are also some spelling errors both in the branch name ("interate") and commit messages ("improved").

These two commits should be squashed, but you can wait with that until you've done the rest of the fixes I suggested. I can help you with that if you want, or @SatjaSivcev might be able to help you.

Finally, the commit from #11 should be part of this PR. Either as a separate commit, or just squashed in with the other changes, that's up to you.

@chrisasc chrisasc force-pushed the interate-over-pointcloud branch from ebe0f54 to f0a9ac8 Compare August 13, 2019 14:12
@chrisasc
Copy link
Contributor Author

Ready for next round

@knatten
Copy link
Contributor

knatten commented Aug 13, 2019

This doesn't build. The file name needs to be capitalized, and CMakeLists.txt needs to be updated to reflect that you renamed the folder and file name.

@knatten
Copy link
Contributor

knatten commented Aug 16, 2019

Let me know when you're ready to start working on this PR again, and I'll help you rebase on master after the formatting change we did in #12.

@SatjaSivcev
Copy link
Contributor

@chrisasc let's look at this next week when I am back. It should be a quick fix.

@knatten
Copy link
Contributor

knatten commented Sep 17, 2019

This needs another rebase after we merge #23

@SatjaSivcev
Copy link
Contributor

@chrisasc now that you are on a C++ coding spree maybe it's a good idea to resolve this branch and merge it?

@SatjaSivcev SatjaSivcev force-pushed the interate-over-pointcloud branch 4 times, most recently from bc68158 to 3f35436 Compare December 1, 2019 19:44
@SatjaSivcev
Copy link
Contributor

I made some fixes here. It builds now and should be ready for merging. @knatten please check if all is good?

@SatjaSivcev SatjaSivcev force-pushed the interate-over-pointcloud branch from 3f35436 to b2206e9 Compare December 1, 2019 20:04
knatten
knatten suggested changes Dec 2, 2019
View reviewed changes
Copy link
Contributor

@knatten knatten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just some nitpicks and a suggested improvement for output.

Zivid/ReadIterateZDF/ReadIterateZDF.cpp Outdated

std::string Filename = "Zivid3D.zdf";
std::cout << "Reading " << Filename << " point cloud" << std::endl;
Zivid::Frame frame = Zivid::Frame(Filename);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be const, and would be good to use the same convention for declaring variables which we use elsewhere:

const Zivid::Frame frame(Filename);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Zivid/ReadIterateZDF/ReadIterateZDF.cpp Outdated
Zivid::CloudVisualizer vis;
zivid.setDefaultComputeDevice(vis.computeDevice());

std::string Filename = "Zivid3D.zdf";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be const

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Zivid/ReadIterateZDF/ReadIterateZDF.cpp Outdated
<< "Height: " << pointCloud.height() << ", Width: " << pointCloud.width() << std::endl;

// Iterating over the point cloud and displaying (X, Y, Z, R, G, B, Contrast)
for(int i = 0; i < pointCloud.height(); i++)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use size_t for i and j since that is what height() and witdh() return

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Zivid/ReadIterateZDF/ReadIterateZDF.cpp Outdated
<< "Height: " << pointCloud.height() << ", Width: " << pointCloud.width() << std::endl;

// Iterating over the point cloud and displaying (X, Y, Z, R, G, B, Contrast)
for(int i = 0; i < pointCloud.height(); i++)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to do i += 100 instead (and the same for j), to avoid extremely long output to console. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Zivid/ReadIterateZDF/ReadIterateZDF.cpp Outdated

std::cout << "Values at pixel (" << i << ", " << j << "):"
<< " X:" << point.x << " Y:" << point.y << " Z:" << point.z
<< " R:" << (int)point.red() << " G:" << (int)point.green() << " B:" << (int)point.blue()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer static_cast over C-style casts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@SatjaSivcev
Copy link
Contributor

SatjaSivcev commented Dec 5, 2019 •
edited
Loading

Fixed comments. However, master has a problem. Maybe rebasing will fix them. But first: ready for next round

@knatten
Copy link
Contributor

knatten commented Dec 6, 2019

However, master has a problem.

Which problem does master have?

Maybe rebasing will fix them. But first: ready for next round

Do you think you could rebase first? It's currently very hard to review this PR due to the git history. Have a look at the diff to understand what I mean.

@SatjaSivcev SatjaSivcev force-pushed the interate-over-pointcloud branch 2 times, most recently from 3ded8cf to aff3a44 Compare December 6, 2019 18:27
knatten
knatten approved these changes Dec 9, 2019
View reviewed changes
Copy link
Contributor

@knatten knatten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good! I think your idea for limiting output is better than mine.

@knatten
Copy link
Contributor

knatten commented Dec 9, 2019

Looks like we're just missing some formatting to be green, then we're good to merge. You can just merge when green, no need for re-approval.

knatten
knatten suggested changes Dec 10, 2019
View reviewed changes
Copy link
Contributor

@knatten knatten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something went wrong in a rebase here, I think?

@chrisasc
Copy link
Contributor Author

Indeed it did! I'm not sure what I did wrong... Could we quickly go through this later today?

@knatten
Copy link
Contributor

knatten commented Dec 10, 2019

Of course! :) Let me know whenever is a good time for you.

@chrisasc chrisasc force-pushed the interate-over-pointcloud branch from 2b88ca8 to 1df9237 Compare December 12, 2019 09:58
@SatjaSivcev SatjaSivcev force-pushed the interate-over-pointcloud branch from 1df9237 to 3c19de4 Compare February 3, 2020 09:55
@SatjaSivcev
Copy link
Contributor

I've rebased this branch. @chrisasc if it is green, plase merge it.

@SatjaSivcev SatjaSivcev force-pushed the interate-over-pointcloud branch from 3c19de4 to 9755b4b Compare February 3, 2020 20:15
@SatjaSivcev
Modify to include point cloud iteration
458f81c 
ReadZDF.cpp is modified and renamed to ReadIterateZDF.cpp
@SatjaSivcev SatjaSivcev force-pushed the interate-over-pointcloud branch from 9755b4b to 458f81c Compare February 3, 2020 20:28
@chrisasc chrisasc merged commit fd2e485 into master Feb 4, 2020
@SatjaSivcev SatjaSivcev deleted the interate-over-pointcloud branch February 8, 2020 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SatjaSivcev SatjaSivcev SatjaSivcev left review comments

@knatten knatten knatten approved these changes

Assignees

@chrisasc chrisasc

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chrisasc @SatjaSivcev @knatten