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

[Visualizer] Added unit test #4016

Closed

Conversation

larshg
Copy link
Contributor

@larshg larshg commented May 2, 2020

Where points get removed and updatepointcloud is called on visualizer.

Basically @MaxFleur MCVE from here.

Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

Small and sweet. Nice 😄

test/visualization/test_visualization.cpp Outdated Show resolved Hide resolved
@kunaltyagi kunaltyagi added module: test needs: more work Specify why not closed/merged yet needs: testing Specify why not closed/merged yet labels May 2, 2020
@kunaltyagi
Copy link
Member

Just a note: There was no test failure. Did the test not run?

@kunaltyagi kunaltyagi added this to the pcl-1.11.0 milestone May 3, 2020
@larshg
Copy link
Contributor Author

larshg commented May 3, 2020

I updated to use the generator.

According to logs, the test is build(Ubuntu, OSx), but it was not run.

I'll have a look after this update and if it is still no run, we'll have to investigate...

Windows builds doesn't have VTK so they won't run it.

@larshg
Copy link
Contributor Author

larshg commented May 3, 2020

MacOS doesn't run the test_visualizer... So the others probably won't do it either... wierd.

@taketwo
Copy link
Member

taketwo commented May 3, 2020

I guess CI does not have DISPLAY environment variable defined, so visualization tests are not built:

if(BUILD_features AND NOT UNIX OR (UNIX AND DEFINED ENV{DISPLAY}))

@kunaltyagi
Copy link
Member

Do all visualization tests require a display? If so, we need to use xvfb (as a hack if this is true only for some tests)

@taketwo
Copy link
Member

taketwo commented May 3, 2020

Well, all sounds too serious. There is a single file with two test cases.

@kunaltyagi
Copy link
Member

kunaltyagi commented May 3, 2020

I don't know how pcl_vis works (only know how it looks from an bird's eye view) 😅 Just adding a virtual buffer for all seemed the fastest way to get the tests running beyond dividing the vis tests in 2 parts.

@taketwo
Copy link
Member

taketwo commented May 3, 2020

Just adding a virtual buffer for all seemed the fastest way to get the tests running

Somebody may give this a try, but I think it's certainly outside of the scope of this PR.

@kunaltyagi
Copy link
Member

kunaltyagi commented May 3, 2020

If anyone can confirm the failure induced due to the PR (or confirm that #4017 indeed solves the issue), we can merge #4017

@taketwo
Copy link
Member

taketwo commented May 3, 2020

I've tried this PR, but the test passes (Ubuntu 18.04 + VTK 6.3).

@larshg
Copy link
Contributor Author

larshg commented May 3, 2020

Maybe its a VTK 8.2 issue only - most users reporting this issue has either been using 8.2 directly or updated to 8.2.

@larshg
Copy link
Contributor Author

larshg commented May 3, 2020

The test is set as being build in the cmake configuration.

but its not build and run in the Run Unit Test.

@taketwo
Copy link
Member

taketwo commented May 3, 2020

The test is set as being build in the cmake configuration.

It's not the test itself, it's the tests_visualization module as a whole. (Which contains a single target test_visualization).

@larshg
Copy link
Contributor Author

larshg commented May 3, 2020

The test is set as being build in the cmake configuration.

It's not the test itself, it's the tests_visualization module as a whole. (Which contains a single target test_visualization).

Ahh, yes ofc.

@kunaltyagi kunaltyagi removed the needs: testing Specify why not closed/merged yet label May 4, 2020
@kunaltyagi kunaltyagi closed this May 6, 2020
@larshg larshg deleted the AddRemovePointAndRerender branch May 8, 2020 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: test needs: more work Specify why not closed/merged yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants