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

Sick Visionary.T.Pro #183

Merged
merged 6 commits into from Mar 12, 2019

Conversation

Projects
None yet
3 participants
@jangernert
Copy link
Collaborator

jangernert commented Feb 18, 2019

No description provided.

@jangernert jangernert requested review from f00f and sisiplac Feb 18, 2019

@sisiplac

This comment has been minimized.

Copy link
Contributor

sisiplac commented Feb 26, 2019

Im am pretty sure that the intrinsics parameters are not interpreted correctly at the moment. Our ProjectiveTransformationZhang represents the Brown-Conrady model, whereas the parameters returned by GetIntrinsics seem to represent the inverse Brown-Conrady model. The topic will be clarified with Sick support team - probably we will need to change our code here.

@sisiplac

This comment has been minimized.

Copy link
Contributor

sisiplac commented Feb 26, 2019

Apart from the intrinsics topic I don't see any problems in the current implementation. Filters should be all deactivated using browser control of the camera, but that is possible. So here is my short todo list which has to be worked through before merge:

  • Await feedback of Sick support team regarding intrinsics and change related code if needed.

@jangernert jangernert changed the title Sick Visionary.T.Pro [WIP] Sick Visionary.T.Pro Feb 26, 2019

Show resolved Hide resolved BetaCameras/Sick.VisionaryT.Pro/CameraModel.cs Outdated
Show resolved Hide resolved BetaCameras/Sick.VisionaryT.Pro/Sick.VisionaryT.Pro.csproj Outdated
Show resolved Hide resolved BetaCameras/Sick.VisionaryT.Pro/Sick.VisionaryT.Pro.csproj Outdated
Show resolved Hide resolved BetaCameras/Sick.VisionaryT.Pro/CameraModel.cs Outdated
// Copyright (c) Metrilus GmbH
// MetriCam 2 is licensed under the MIT license. See License.txt for full license text.

using System;

This comment has been minimized.

@f00f

f00f Feb 27, 2019

Contributor

using statements should be sorted alphabetically.

Show resolved Hide resolved BetaCameras/Sick.VisionaryT.Pro/VisionaryTPro.cs Outdated
Show resolved Hide resolved BetaCameras/Sick.VisionaryT.Pro/VisionaryTPro.cs Outdated
Show resolved Hide resolved BetaCameras/Sick.VisionaryT.Pro/VisionaryTPro.cs Outdated
Show resolved Hide resolved BetaCameras/Sick.VisionaryT.Pro/VisionaryTPro.cs Outdated
Show resolved Hide resolved BetaCameras/Sick.VisionaryT.Pro/VisionaryTPro.cs Outdated

@jangernert jangernert force-pushed the sick_pro branch 2 times, most recently from 41ad72a to 924e253 Feb 27, 2019

Show resolved Hide resolved BetaCameras/Sick.VisionaryT.Pro/VisionaryTPro.cs Outdated

@jangernert jangernert force-pushed the sick_pro branch 2 times, most recently from 1c330d3 to b17939b Mar 5, 2019

@jangernert jangernert force-pushed the sick_pro branch from 6545e6b to 748e14d Mar 5, 2019

@Metrilus Metrilus deleted a comment from jangernert Mar 5, 2019

@f00f

This comment has been minimized.

Copy link
Contributor

f00f commented Mar 5, 2019

One of the force-pushes reverted changes made by me (use override instead of new, add Vendor to Visionary-T) and @sisiplac (look for InverseBrownConradyParams - it's gone).

I have created a new branch from my working copy, rebased to master: sick_pro_unforced. You may use that to recover a working implementation and redo your changes afterwards.

Please add your changes as new commits, so reviewers have a chance of keeping track.
No need to take care of the history, it's messed up already, and it will be squashed during the merge anyways into a single commit named "Add Sick Visionary-T Pro".

If the remote branch contains changes, rebase your changes onto that before pushing:

git push // fails because `origin` has other commits
git log  // note the hashes of your new commits
git fetch
git reset --hard origin/sick_pro   // undoes all your changes
git cherry-pick <your commit hash> // for all your new commits
git push

(I'll add this to our wiki, too)

@sisiplac

This comment has been minimized.

Copy link
Contributor

sisiplac commented Mar 6, 2019

From my side only the commit including the InverseBrownConrady changes was missing. I cherry-picked it from the new branch created by Hannes and had another (positive) live test with the camera.

f00f and others added some commits Mar 1, 2019

@jangernert jangernert force-pushed the sick_pro branch from 97b761e to 2dad023 Mar 6, 2019

@jangernert jangernert requested a review from f00f Mar 6, 2019

@f00f

f00f approved these changes Mar 12, 2019

@f00f

This comment has been minimized.

Copy link
Contributor

f00f commented Mar 12, 2019

Still has WIP in the title, and the wip-label. Therefore, I will not merge. But review is ok.

@jangernert

This comment has been minimized.

Copy link
Collaborator Author

jangernert commented Mar 12, 2019

@sisiplac and I have nothing left planned for now. So I'll remove the [WIP] and merge to master.

@jangernert jangernert changed the title [WIP] Sick Visionary.T.Pro Sick Visionary.T.Pro Mar 12, 2019

@jangernert jangernert merged commit 438f7a8 into master Mar 12, 2019

1 check passed

MetriCam2 CI Build successful
Details

@jangernert jangernert deleted the sick_pro branch Mar 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.