Skip to content

Fix/18.04#241

Merged
floriantschopp merged 10 commits intomasterfrom
fix/18.04
Feb 3, 2020
Merged

Fix/18.04#241
floriantschopp merged 10 commits intomasterfrom
fix/18.04

Conversation

@floriantschopp
Copy link
Copy Markdown
Contributor

@floriantschopp floriantschopp commented Nov 13, 2018

Make it compile on 18.04 (can someone on 16.04 test this please?)

Related issues are:

@vilvo
Copy link
Copy Markdown

vilvo commented Nov 13, 2018

I tested Kalibr in an Ubuntu 18.04 LXC container with the same change a few weeks back. Works fine.
One input from that test is that LXC runs in non-windowing environment so the PDF reports do not get generated. Would be nice to have them even without displaying the PDF results.

@andre-nguyen
Copy link
Copy Markdown

andre-nguyen commented Jan 12, 2019

👍 This is needed for multicam calibration to work.

HannesSommer
HannesSommer reviewed Feb 11, 2019
View reviewed changes
Comment thread aslam_offline_calibration/kalibr/python/kalibr_camera_calibration/MulticamGraph.py Outdated
@HannesSommer
Update MulticamGraph.py
d8fc0e2 
Preserve some backward compatibility with older python versions.
@HannesSommer
Copy link
Copy Markdown
Contributor

HannesSommer commented Feb 17, 2019

Has somebody tested this on 16.04 in the meantime?

@floriantschopp floriantschopp requested a review from smauq October 4, 2019 07:20
@helenol
Copy link
Copy Markdown
Contributor

helenol commented Jan 17, 2020 •
edited
Loading

Can we merge this now that it's 2020? :) Jenkins says it builds on 16.04 in case anyone is still in the dark ages.

@floriantschopp
Copy link
Copy Markdown
Contributor Author

floriantschopp commented Jan 17, 2020

OK with me :-)

@HannesSommer
Copy link
Copy Markdown
Contributor

HannesSommer commented Feb 2, 2020

Apparently nobody feels really responsible for this anymore. I'd suggest to merge this without the add option to manually input forcal length, which doesn't fit this PR and has possibly bad implications for people using this as part of unsupervised scripts. Also, it has an indentation problem and a comment was not properly moved, but that we could address in a separate PR. I know it was an MR before into this branch but that is also bad for the history and I wouldn't want to accept that part in the current state.

@floriantschopp , do you have time to quickly split the two? Otherwise, I can do it, but that would obfuscate authorship.

@floriantschopp
Copy link
Copy Markdown
Contributor Author

floriantschopp commented Feb 2, 2020

@HannesSommer Will do tomorrow 👍

@floriantschopp
Revert "Merge pull request #304 from ethz-asl/feature/input_focal_len…
aac34c2 
…gth_guess"

This reverts commit 2999e18, reversing
changes made to ee2b09d.
@floriantschopp floriantschopp mentioned this pull request Feb 3, 2020
Merged
HannesSommer
HannesSommer approved these changes Feb 3, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@HannesSommer HannesSommer left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM. We should not forget to squash the history.

@floriantschopp floriantschopp merged commit cb6fb66 into master Feb 3, 2020
@ruffsl
Copy link
Copy Markdown

ruffsl commented Feb 6, 2020

Speaking of building on Ubuntu 18.04, perhaps someone could checkout the new PR adding a Dockerfile for reproducible builds, as I'm not sure how to fix the includes sm_common package: #347 (comment)

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

Reviewers

@ZacharyTaylor ZacharyTaylor Awaiting requested review from ZacharyTaylor

@schneith schneith Awaiting requested review from schneith

@smauq smauq Awaiting requested review from smauq

2 more reviewers

@andrepfr andrepfr andrepfr left review comments

@HannesSommer HannesSommer HannesSommer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@floriantschopp @vilvo @andre-nguyen @HannesSommer @helenol @ruffsl @andrepfr