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

Orbbec improvements #168

Merged
merged 12 commits into from Nov 13, 2018
Merged

Orbbec improvements #168

merged 12 commits into from Nov 13, 2018

Conversation

f00f
Copy link
Contributor

@f00f f00f commented Nov 13, 2018

This makes MetriCam's Orbbec wrapper compatible with current Orbbec Astra Mini S devices - and hopefully other current devices, too.

Suggested commit message for the squash commit:

Orbbec improvements

- Compatibility with current devices
- Cleaner OpenNI2 init/shutdown
- Faster device discovery
- Several minor improvements

@f00f
Copy link
Contributor Author

f00f commented Nov 13, 2018

I've also successfully tested the MinimalSample of this branch with 2 cameras now. \o/

@f00f
Copy link
Contributor Author

f00f commented Nov 13, 2018

I'm unsure about including b5ad238, the changes to the MinimalSample. What's your opinion?

@jangernert
Copy link
Collaborator

@f00f the only thing I don't like about the minimal sample changes is that it now makes some assumptions specific to orbbec cameras like // This should throw
But on the other hand the MinimalSample is more or less just a playground used while working on a camera back-end. So one will have stashed local changes to it all the time anyway.

@f00f
Copy link
Contributor Author

f00f commented Nov 13, 2018

@f00f the only thing I don't like about the minimal sample changes is that it now makes some assumptions specific to orbbec cameras like // This should throw
But on the other hand the MinimalSample is more or less just a playground used while working on a camera back-end. So one will have stashed local changes to it all the time anyway.

Fully agree. It would be cool to have this separated, i.e. a generic file and a file that runs if camerais Orbbecetc. That reminds me of mycamera-tests` branch...

bool initSucceeded = OpenNIInit();
if (!initSucceeded)
{
log->Error("Could not initialize OpenNI");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the Astra camera operational if initializing OpenNI failed? If not, just logging an error seems not harsh enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Changed in 8aad6ff

@jangernert jangernert merged commit 74d347e into master Nov 13, 2018
@jangernert jangernert deleted the orbbec-improvements branch November 13, 2018 20:51
@sisiplac
Copy link
Contributor

I would have liked the merge commit message to contain the info that our implementation is no longer compatible with blue cameras. For people working with blue cameras the change in the IR offset can be breaking.

@f00f
Copy link
Contributor Author

f00f commented Nov 23, 2018

@sisiplac if it was ever applied in the correct direction...
But you are right. The changelog of the next release should mention that as a breaking change.

@sisiplac
Copy link
Contributor

@f00f I can definitively confirm that it was applied in the correct direction for devices with blue PCB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Orbbec
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

3 participants