Temporary bugfix for issue #4082 #4151

OgreTransporter · 2020-05-29T09:26:40Z

Temporary bugfix for issue #4082 by undoing parts of commit 02ad0b6

kunaltyagi · 2020-05-29T09:28:51Z

Please undo both commits (#3972, #3971) and add a todo note about removing them

OgreTransporter · 2020-05-29T09:34:53Z

You want me to undo both PR completely? I only have problems with commit 02ad0b6, the rest works fine for me.

kunaltyagi · 2020-05-29T09:45:54Z

Yes. @SunBlack showed that both PR created the same issue for Windows users.

Moreover, would it be possible to create a test with instantiation of these templates so that when in future we try to fix the todo, we don't accidentally recreate this problem?

OgreTransporter · 2020-05-29T10:48:43Z

New tests are not necessary. The tests test_convolution (filters) and test_poisson (surface) call these templates.

kunaltyagi · 2020-05-29T11:19:03Z

No tests have been failing on master. That means there's some difference in the setup on CI and on the system of at least 3 people. We need to replicate that difference

OgreTransporter · 2020-05-29T11:47:31Z

The patch from above is sufficient that tools and tests can be compiled on Windows at my site - which was the original problem. You would have to ask the others if the patch is sufficient there. Then we have to compare the different systems.

Var	CI	OgreTransporter
OS	Windows Server 2016 Datacenter	Windows 10
VS	2017 x64	2017 x64
MSVC	19.16.27040.0	19.16.27040.0
Eigen	3.3.7	3.3.90
flann	1.9.1	1.9.1
libusb	-	1.0.23
ZLIB	1.2.11	1.2.11
PNG	-	1.6.38
Qhull	?	7.3.2
Boost	1.69	1.74
GLEW	-	2.2.0
CUDA	-	10.2
Tests	119/119	117/117

kunaltyagi · 2020-05-29T12:03:12Z

Pinging @UnaNancyOwen and @SunBlack for this

UnaNancyOwen · 2020-05-29T14:17:02Z

I have confirmed that this can be built in my environment. 👍

kunaltyagi · 2020-05-29T15:58:29Z

@UnaNancyOwen Why does the CI miss this? Could you please investigate that so we can have increased confidence in the CI?

Waiting for sunblack to confirm, because his setup showed errors from both PRs

UnaNancyOwen · 2020-05-29T22:50:09Z

Why does the CI miss this? Could you please investigate that so we can have increased confidence in the CI?

In CI, Tools is disabled on Windows. (I don't remember the exact reason for it, but) I think it's because it takes longer or has a larger storage capacity.

pcl/.ci/azure-pipelines/build/windows.yaml

Line 32 in 149e0dd

-DBUILD_tools=OFF ^

SunBlack · 2020-05-29T22:55:01Z

My build result with this branch:

UnaNancyOwen · 2020-05-29T23:55:24Z

My build result with this branch:

I think it's related to this issue #1951.
It occuler when BUILD_examples is turned on. (It option is OFF in default.)

kunaltyagi · 2020-05-30T05:40:57Z

@UnaNancyOwen Apps and Tools are off on CI because of VTK. Installing VTK itself takes 1 hour.
SunBlack has 1 error due to Compile error relative to example_nurbs_viewer_surface #1951 but 2 which can be undone here IIRC
2 errors are due to apps module. Please confirm this and afterwards, @OgreTransporter can undo the second PR too

@SunBlack Can you confirm test the presence of #1952 on pcl-1.10 release? (Just to segregate the 2 errors)

larshg · 2020-05-30T07:25:43Z

From what I can gather, the test_convolution is not added to any test projects, so its not used atm. Also, it doesn't contain any tests which uses the pcl::RGB structs which seems to cause trouble.

OgreTransporter · 2020-05-31T11:00:01Z

This problem is very strange, especially because the Visual Studio versions behave so differently on different computers - although they should be identical.

From what I can gather, the test_convolution is not added to any test projects, so its not used atm. Also, it doesn't contain any tests which uses the pcl::RGB structs which seems to cause trouble.

I have the test included. But yes, you're right, pcl::RGB is not actually called, pcl::PointXYZI is called. My fault.

I'm building all the tools. But without VTK, because VTK 9 is not yet supported.

kunaltyagi

inline on all instances please.

And please add a TODO stating intent to move to cpp file on all the instances moved back to header files

surface/include/pcl/surface/3rdparty/poisson4/bspline_data.hpp

kunaltyagi · 2020-05-31T11:16:49Z

There's a WIP for VTK 9 support if you're inclined. Could you also

Add a simple test which merely calls these functions
Reorganize commits so that the test case addition is 1st commit
Verify test case fails with additional cmake parameters: -DWITH_VTK:BOOL=OFF -DBUILD_global_tests:BOOL=ON?

OgreTransporter · 2020-05-31T12:22:22Z

CMake:

cmake -G "Visual Studio 15" -A x64 -D CMAKE_INSTALL_PREFIX:PATH=D:\test\PCL\install -D CMAKE_DEBUG_POSTFIX:STRING=d -D WITH_DOCS:BOOL=OFF -D BUILD_CUDA:BOOL=ON -D BUILD_cuda_io:BOOL=ON -D BUILD_global_tests:BOOL=ON -D EIGEN_INCLUDE_DIR:PATH=D:\test\PCL\deps\Eigen\install\include\eigen3 -D FLANN_INCLUDE_DIR:PATH=D:\test\PCL\deps\FLANN\install\include -D FLANN_LIBRARY:FILEPATH=D:\test\PCL\deps\FLANN\install\lib\flann_s.lib -D FLANN_LIBRARY_DEBUG:FILEPATH=D:\test\PCL\deps\FLANN\install\lib\flann_sd.lib -D ZLIB_INCLUDE_DIR:PATH=D:\test\PCL\deps\zlib\install\include -D ZLIB_LIBRARY_DEBUG:FILEPATH=D:\test\PCL\deps\zlib\install\lib\zlibstaticd.lib -D ZLIB_LIBRARY_RELEASE:FILEPATH=D:\test\PCL\deps\zlib\install\lib\zlibstatic.lib -D GLEW_DIR:PATH=D:\test\PCL\deps\GLEW\install -D GLEW_INCLUDE_DIR:PATH=D:\test\PCL\deps\GLEW\install\include -D GLEW_SHARED_LIBRARY_DEBUG:FILEPATH=D:\test\PCL\deps\GLEW\install\lib\glewd.lib -D GLEW_SHARED_LIBRARY_RELEASE:FILEPATH=D:\test\PCL\deps\GLEW\install\lib\glew.lib -D PNG_PNG_INCLUDE_DIR:PATH=D:\test\PCL\deps\libpng\install\include -D PNG_LIBRARY_DEBUG:FILEPATH=D:\test\PCL\deps\libpng\install\lib\libpng16_staticd.lib -D PNG_LIBRARY_RELEASE:FILEPATH=D:\test\PCL\deps\libpng\install\lib\libpng16_static.lib -D Boost_INCLUDE_DIR:PATH=D:\test\PCL\deps\boost\install -D LZ4_LIBRARIES:FILEPATH=debug;D:\test\PCL\deps\lz4\install\lib\Debug\liblz4_static.lib;optimized;D:\test\PCL\deps\lz4\install\lib\Release\liblz4_static.lib -D LZ4_INCLUDE_DIR:PATH=D:\test\PCL\deps\lz4\install\include -D QHULL_LIBRARY:FILEPATH=D:\test\PCL\deps\Qhull\install\lib\qhullstaticd.lib -D QHULL_LIBRARY_DEBUG:FILEPATH=D:\test\PCL\deps\Qhull\install\lib\qhullstatic.lib -D LIBUSB_1_INCLUDE_DIR:PATH=D:\test\PCL\deps\libusb\install\include -D LIBUSB_1_LIBRARY:FILEPATH=debug;D:\test\PCL\deps\libusb\install\lib\libusbs-1.0d.lib;optimized;D:\test\PCL\deps\libusb\install\lib\libusbs-1.0.lib -D GTEST_INCLUDE_DIR:PATH=D:\test\PCL\deps\googletest\install\include -D GTEST_LIBRARY:FILEPATH=D:\test\PCL\deps\googletest\install\lib\gtest.lib -D GTEST_LIBRARY_DEBUG:FILEPATH=D:\test\PCL\deps\googletest\install\lib\gtestd.lib -D GTEST_MAIN_LIBRARY:FILEPATH=D:\test\PCL\deps\googletest\install\lib\gtest_main.lib -D GTEST_MAIN_LIBRARY_DEBUG:FILEPATH=D:\test\PCL\deps\googletest\install\lib\gtest_maind.lib -D GTEST_SRC_DIR:PATH=D:\test\PCL\googletest\googletest -D Qt5_DIR:PATH=D:\dev\SDKs\Qt5\5.14.1\msvc2017_64\lib\cmake\Qt5 -D "CUDA_SDK_ROOT_DIR:PATH=C:\Program Files\NVIDIA GPU Computing Toolkit\CUDA\v10.2" D:\test\PCL\src

Result:

-- The following subsystems will be built:
--   common
--   kdtree
--   octree
--   search
--   sample_consensus
--   filters
--   2d
--   geometry
--   io
--   features
--   ml
--   segmentation
--   surface
--   registration
--   keypoints
--   tracking
--   recognition
--   stereo
--   cuda_common
--   cuda_io
--   cuda_features
--   cuda_segmentation
--   cuda_sample_consensus
--   global_tests
--   tests_2d
--   tests_common
--   tests_features
--   tests_filters
--   tests_geometry
--   tests_io
--   tests_kdtree
--   tests_keypoints
--   tests_octree
--   tests_recognition
--   tests_registration
--   tests_search
--   tests_surface
--   tests_segmentation
--   tests_sample_consensus
--   tools
-- The following subsystems will not be built:
--   visualization: VTK was not found.
--   apps: Disabled by default
--   cuda_apps: VTK was not found.
--   outofcore: Disabled manually.
--   examples: Code examples are disabled by default.
--   people: Disabled manually.
--   simulation: Disabled by default.
--   tests_people: Requires visualization.
--   tests_outofcore: Requires visualization.
--   tests_visualization: Requires visualization.

Build result (Debug and Release): 197 projects successful

Test result:

1>100% tests passed, 0 tests failed out of 117
1>
1>Total Test time (real) = 140.77 sec

In detail:

1>test 57
1>        Start  57: filters_convolution
1>
1>57: Test command: D:\test\PCL\build\bin\test_convolution.exe
1>57: Test timeout computed to be: 10000000
1>57: [==========] Running 2 tests from 1 test suite.
1>57: [----------] Global test environment set-up.
1>57: [----------] 2 tests from Convolution
1>57: [ RUN      ] Convolution.convolveRows
1>57: [       OK ] Convolution.convolveRows (0 ms)
1>57: [ RUN      ] Convolution.convolveRowsRGB
1>57: [       OK ] Convolution.convolveRowsRGB (1 ms)
1>57: [----------] 2 tests from Convolution (1 ms total)
1>57:
1>57: [----------] Global test environment tear-down
1>57: [==========] 2 tests from 1 test suite ran. (1 ms total)
1>57: [  PASSED  ] 2 tests.
1> 57/117 Test  #57: filters_convolution .................................   Passed    0.04 sec

1>test 110
1>        Start 110: surface_poisson
1>
1>110: Test command: D:\test\PCL\build\bin\test_poisson.exe "D:/test/PCL/src/test/bun0.pcd"
1>110: Test timeout computed to be: 10000000
1>110: [==========] Running 1 test from 1 test suite.
1>110: [----------] Global test environment set-up.
1>110: [----------] 1 test from PCL
1>110: [ RUN      ] PCL.Poisson
1>110: [       OK ] PCL.Poisson (267 ms)
1>110: [----------] 1 test from PCL (267 ms total)
1>110:
1>110: [----------] Global test environment tear-down
1>110: [==========] 1 test from 1 test suite ran. (267 ms total)
1>110: [  PASSED  ] 1 test.
1>110/117 Test #110: surface_poisson .....................................   Passed    0.31 sec

Reorganize commits so that the test case addition is 1st commit

So I should create a new PR with a different commit order?

FYI: VTK takes me only 25 minutes to build in debug and release. Why the installation takes an hour is a mystery to me. The longest time for me is OpenCV with four hours and 16 minutes. All other libraries and programs I build here are faster.

kunaltyagi · 2020-05-31T17:04:12Z

So I should create a new PR with a different commit order?

No need. You can use git rebase -i <root_of_your_branch> where the root might be master. Then you can simply move the lines (commits) around

VTK takes me only 25 minutes to build

That's your system. CI machines might be a bit slower (at least the IO is heavily throttled)

Also, your test needs refactoring. Use loops and minimize the size as much as possible.

OgreTransporter · 2020-06-02T10:50:14Z

Add a simple test which merely calls these functions

I duplicated the original test in test_convolution and adapted the data to form an RGB point cloud. This is how the functions are called. The original test is also provided with "inline" data. What should I change there? The other template functions are called in test test_poisson.

Reorganize commits so that the test case addition is 1st commit

Done.

Verify test case fails with additional cmake parameters: -DWITH_VTK:BOOL=OFF -DBUILD_global_tests:BOOL=ON?

Also the CI can perform the test without problems, as I have seen: https://dev.azure.com/PointCloudLibrary/pcl/_build/results?buildId=16518&view=logs&j=d944f354-b9eb-5c76-5ade-3c70deb27a9e&t=e6a92e69-5f1c-567d-0c8e-3e6b9c56a5da&l=1718

test/filters/test_convolution.cpp

…4082

OgreTransporter · 2020-06-03T06:22:06Z

Fixed and rebase

larshg · 2020-06-03T08:12:21Z

Ohh my, how can things be so simple... yet sooo far away from memory :)

I figured it out after reading this post https://stackoverflow.com/a/56801030.

We need to add PCL_EXPORTS to the offending functions. I would recon that all the functions that was moved to the .cpp will require this.

Alternatively we can add PCL_EXPORTS to the entire Convolution class - not sure what granularity we would like?

@OgreTransporter I think if you add a unit test for the PointXYZRGB as well, it will also fail, if we don't do the above.

kunaltyagi · 2020-06-03T09:56:57Z

I would recon that all the functions that was moved to the .cpp will require this

Ofc, PCL_EXPORT would solve this. Wow. Nice find. MSVC behavior makes much more sense now

OgreTransporter · 2020-06-04T10:05:46Z

@larshg Many thanks for the solution!

I have created a new PR #4165 to test this solution separately. I have extended the unit test accordingly.

kunaltyagi · 2020-06-04T14:01:53Z

Closing in favor of #4165

OgreTransporter mentioned this pull request May 29, 2020

[windows] symbol not found: filters::Convolution<RGB,RGB>::convolveOne{Row,Col}Dense() #4082

Closed

kunaltyagi added changelog: fix Meta-information for changelog generation module: filters module: surface needs: more work Specify why not closed/merged yet labels May 29, 2020

kunaltyagi linked an issue May 29, 2020 that may be closed by this pull request

[windows] symbol not found: filters::Convolution<RGB,RGB>::convolveOne{Row,Col}Dense() #4082

Closed

kunaltyagi added needs: feedback Specify why not closed/merged yet platform: windows and removed needs: more work Specify why not closed/merged yet labels May 29, 2020

kunaltyagi requested changes May 31, 2020

View reviewed changes

surface/include/pcl/surface/3rdparty/poisson4/bspline_data.hpp Outdated Show resolved Hide resolved

kunaltyagi added the needs: more work Specify why not closed/merged yet label May 31, 2020

OgreTransporter requested a review from kunaltyagi June 2, 2020 11:32

kunaltyagi requested changes Jun 2, 2020

View reviewed changes

test/filters/test_convolution.cpp Outdated Show resolved Hide resolved

OgreTransporter added 4 commits June 3, 2020 08:18

Extend the convolution test (PointXYZI) with an RGB version.

955ee32

Temporary bugfix for issue #4082 by undoing parts of commit 02ad0b6

834f134

Temporary bugfix for issue #4082 by undoing parts of commit 68aba0c

7ddb3df

Add todo information to move functions to cpp files considering issue #…

6db2bbf

…4082

OgreTransporter mentioned this pull request Jun 4, 2020

Fix missing symbol issue on Windows for filters::convolution and poisson4::bspline #4165

Merged

kunaltyagi closed this Jun 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Temporary bugfix for issue #4082 #4151

Temporary bugfix for issue #4082 #4151

OgreTransporter commented May 29, 2020

kunaltyagi commented May 29, 2020

OgreTransporter commented May 29, 2020

kunaltyagi commented May 29, 2020

OgreTransporter commented May 29, 2020

kunaltyagi commented May 29, 2020

OgreTransporter commented May 29, 2020 •

edited

Loading

kunaltyagi commented May 29, 2020

UnaNancyOwen commented May 29, 2020

kunaltyagi commented May 29, 2020

UnaNancyOwen commented May 29, 2020

SunBlack commented May 29, 2020

UnaNancyOwen commented May 29, 2020

kunaltyagi commented May 30, 2020 •

edited

Loading

larshg commented May 30, 2020

OgreTransporter commented May 31, 2020

kunaltyagi left a comment

kunaltyagi commented May 31, 2020

OgreTransporter commented May 31, 2020

kunaltyagi commented May 31, 2020

OgreTransporter commented Jun 2, 2020

OgreTransporter commented Jun 3, 2020

larshg commented Jun 3, 2020

kunaltyagi commented Jun 3, 2020

OgreTransporter commented Jun 4, 2020 •

edited

Loading

kunaltyagi commented Jun 4, 2020

Temporary bugfix for issue #4082 #4151

Temporary bugfix for issue #4082 #4151

Conversation

OgreTransporter commented May 29, 2020

kunaltyagi commented May 29, 2020

OgreTransporter commented May 29, 2020

kunaltyagi commented May 29, 2020

OgreTransporter commented May 29, 2020

kunaltyagi commented May 29, 2020

OgreTransporter commented May 29, 2020 • edited Loading

kunaltyagi commented May 29, 2020

UnaNancyOwen commented May 29, 2020

kunaltyagi commented May 29, 2020

UnaNancyOwen commented May 29, 2020

SunBlack commented May 29, 2020

UnaNancyOwen commented May 29, 2020

kunaltyagi commented May 30, 2020 • edited Loading

larshg commented May 30, 2020

OgreTransporter commented May 31, 2020

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi commented May 31, 2020

OgreTransporter commented May 31, 2020

kunaltyagi commented May 31, 2020

OgreTransporter commented Jun 2, 2020

OgreTransporter commented Jun 3, 2020

larshg commented Jun 3, 2020

kunaltyagi commented Jun 3, 2020

OgreTransporter commented Jun 4, 2020 • edited Loading

kunaltyagi commented Jun 4, 2020

OgreTransporter commented May 29, 2020 •

edited

Loading

kunaltyagi commented May 30, 2020 •

edited

Loading

OgreTransporter commented Jun 4, 2020 •

edited

Loading