Skip to content
This repository was archived by the owner on Jan 21, 2022. It is now read-only.

Add lidar point cloud to visualization of sensor data - #32

Open
MEpping wants to merge 5 commits into
OpenSimulationInterface:masterfrom
MEpping:lidar-point-cloud
Open

Add lidar point cloud to visualization of sensor data#32
MEpping wants to merge 5 commits into
OpenSimulationInterface:masterfrom
MEpping:lidar-point-cloud

Conversation

@MEpping

Copy link
Copy Markdown
Contributor

I split up my previous pull request, so that now this pull request contains the point cloud only (while the other one contains only the win32 fixes). I'm sorry for the confusion.

@pmai

pmai commented Sep 29, 2018

Copy link
Copy Markdown
Contributor

This seems to add files glpointcloud.cpp and pointcloud.cpp, but does not add them to the relevant CMakeLists.txt file, which causes the build failure seen in the CI:

CMakeFiles/osi-visualizer.dir/src/glwidget.cpp.o: In function `GLWidget::GLWidget(QWidget*, IMessageSource*, QMap<ObjectType, QTreeWidgetItem*>&, AppConfig const&)':
/project/src/glwidget.cpp:49: undefined reference to `PointCloud::PointCloud(int, QOpenGLFunctions_4_3_Core*, QString const&)'
CMakeFiles/osi-visualizer.dir/src/glwidget.cpp.o: In function `GLWidget::MessageParsed(QVector<MessageStruct> const&, QVector<LaneStruct> const&, QVector<PointStruct> const&)':
/project/src/glwidget.cpp:605: undefined reference to `PointCloud::UpdatePointCloud(QVector<PointStruct> const&)'
collect2: error: ld returned 1 exit status
CMakeFiles/osi-visualizer.dir/build.make:701: recipe for target 'osi-visualizer' failed
make[2]: *** [osi-visualizer] Error 1
CMakeFiles/Makefile2:68: recipe for target 'CMakeFiles/osi-visualizer.dir/all' failed
make[1]: *** [CMakeFiles/osi-visualizer.dir/all] Error 2
Makefile:129: recipe for target 'all' failed
make: *** [all] Error 2

Please fix this...

@haoyuanying

Copy link
Copy Markdown
Contributor

The Y in OSI stands for the left direction in bird view (X points to the sensor 0° direction, by default, on screen the red triangle points to).

@MEpping

Copy link
Copy Markdown
ContributorAuthor

@haoyuanying: Are you saying the coordinate system is different from my comment: x: up, y: out of monitor, z: right? But your statement that Y points left on the screen cannot be true, because we set the y component to 0 and it works just fine. Maybe you mean that azimuth points counter-clockwise? That would be up to debate, but I think in the automotive context it is more common to interpret it clockwise...

@haoyuanying

Copy link
Copy Markdown
Contributor

The Y I mentioned is in OSI, to transfer the Y in GL, it should be placed on the Z (GL) position...

@MEpping

Copy link
Copy Markdown
ContributorAuthor

@haoyuanying I agree on your orientation of the coordinate system. I'm confused why you think your change of the sign is correct. Consider the following example, let me know at which point you disagree: "left" would be (0, 1, 0) in "OSI-coordinates". This corresponds to azimuth = -90, elevation = 0. This should be (0,0,-1) in "GL-coordinates". My original code gives (0,0,-1). Your code gives (0,0,1).

@haoyuanying

Copy link
Copy Markdown
Contributor

Hello, from
"left" would be (0, 1, 0) in "OSI-coordinates". This corresponds to azimuth = -90, elevation = 0."
The left hand should be +90° in azimuth, in OSI.

@MEpping

Copy link
Copy Markdown
ContributorAuthor

@haoyuanying Ok, so you are saying that azimuth is defined to be pointing counter-clockwise in OSI. While this is the usual definition in mathematics it is quite uncommon in navigation. Could you please point me to the relevant paragraph in the documentation? Because I was wondering about this issue before. If it's not defined in the documentation your change should be reversed.
Would be interesting to hear the opinion of long time contributors like @pmai on this, too.

@haoyuanying

Copy link
Copy Markdown
Contributor

@MEpping
Please read through the osi_common.proto, before the definition of Orientation3D:
// The preferred angular range is (-pi, pi]. The coordinate system is defined as
// right-handed.
// For the sense of each rotation, the right-hand rule applies.

@MEpping

Copy link
Copy Markdown
ContributorAuthor

@haoyuanying Ok, I agree that the flipped sign follows the description in the proto-files. This will produce a lot of confusion, because, as I said, in navigation azimuth usually points in the other direction. See e.g. https://en.wikipedia.org/wiki/Azimuth. However, since the counter-clockwise definition is at least as popular we run into these problems anyways, I'm afraid. So we can stick with your code and change the sign of azimuth on the sending side where necessary.

@haoyuanying

Copy link
Copy Markdown
Contributor

The comments in the proto files are Doxygen compliant, and, they should be treated as official documentation of OSI...

@ghost
ghost requested a review from pmaiJanuary 30, 2019 18:01
@ghostghost added the feature request Proposals which enhance the interface or add additional features. label Jan 30, 2019
@ghost

ghost commented Apr 4, 2019

Copy link
Copy Markdown

@MEpping I am back again from parental leave and can pick up where we left. I can offer to schedule a call here?

@jdsika
jdsika requested review from jdsika and removed request for pmaiSeptember 19, 2019 09:16

@jdsikajdsika left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@vkresch please test before merge. I just approve in order not to block your work here as Altran intends to contribute actively to this open source tool.

@vkresch

Copy link
Copy Markdown
Contributor

For now this branch is paused because it didn't work with the newest osi-visualizer changes. Feel free to update the branch feature to the stable version of osi-visualizer.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

feature requestProposals which enhance the interface or add additional features.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@MEpping@pmai@haoyuanying@vkresch@jdsika