This fix for cases where VERTEX and NROMAL share same polylist <p> , see https://github.com/osrf/gazebo/issues/2682#issue-602951524 #2811
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ORMAL) * vcount.size() * 3. If both VERTEX and NORMAL have offset 0, then the length of polylist <p> is vcount.size() * 3. So use length of set(offset) as inputSize. This fix for cases where VERTEX and NROMAL share same vcounts, see gazebosim#2682 (comment)
iche033
reviewed
Aug 11, 2020
gazebo/common/ColladaLoader.cc
Outdated
| for (const auto &input : inputs) | ||
| inputSize += input.second.size(); | ||
| std::set<int> total_inputs; | ||
| for (const auto &input : inputs) { |
There was a problem hiding this comment.
minor coding style nitpick:
total_inputs->totalInputs- bracket
{on new line
| @@ -28,6 +28,10 @@ class ColladaVisualization : public QTestFixture | |||
| /// \brief Test loading a collada mesh that has multiple texture | |||
| /// coordinates | |||
| private slots: void MultipleTextureCoordinates(); | |||
|
|
|||
| /// \brief Test loading a collada mesh that has multiple inputs | |||
| /// with samee offset, see https://github.com/osrf/gazebo/issues/2682 | |||
| <!-- <uri>file://media/materials/scripts/gazebo.material</uri> --> | ||
| <!-- <name>Gazebo/Red</name> --> | ||
| <!-- </script> --> | ||
| <!-- </material> --> |
There was a problem hiding this comment.
can you remove the commented out lines?
|
Thanks. Fix and updated.
--
◉ Kei Okada
2020年8月11日(火) 12:14 Ian Chen <notifications@github.com>:
… ***@***.**** commented on this pull request.
changes look good to me! I think we may run into the same issue with
triangles here
<https://github.com/osrf/gazebo/blob/e07009866f649e94fdc7d5c355cb1598504c819a/gazebo/common/ColladaLoader.cc#L1921>
but need to confirm. Other than that I just have minor coding style
comments.
Thanks for the contribution.
------------------------------
In gazebo/common/ColladaLoader.cc
<#2811 (comment)>:
> @@ -1595,8 +1595,11 @@ void ColladaLoader::LoadPolylist(TiXmlElement *_polylistXml,
polylistInputXml = polylistInputXml->NextSiblingElement("input");
}
- for (const auto &input : inputs)
- inputSize += input.second.size();
+ std::set<int> total_inputs;
+ for (const auto &input : inputs) {
minor coding style nitpick:
- total_inputs -> totalInputs
- bracket { on new line
------------------------------
In test/integration/collada_visualization.hh
<#2811 (comment)>:
> @@ -28,6 +28,10 @@ class ColladaVisualization : public QTestFixture
/// \brief Test loading a collada mesh that has multiple texture
/// coordinates
private slots: void MultipleTextureCoordinates();
+
+ /// \brief Test loading a collada mesh that has multiple inputs
+ /// with samee offset, see #2682
samee -> same
------------------------------
In test/worlds/multiple_input_with_same_offset_test.world
<#2811 (comment)>:
> + <!-- </script> -->
+ <!-- </material> -->
+ </visual>
+ <visual name="visual-assimp-4">
+ <pose>0 -5 0.5 0 0 0</pose>
+ <geometry>
+ <mesh>
+ <uri>file://media/models/box-assimp-4.dae</uri>
+ </mesh>
+ </geometry>
+ <!-- <material> -->
+ <!-- <script> -->
+ <!-- <uri>file://media/materials/scripts/gazebo.material</uri> -->
+ <!-- <name>Gazebo/Red</name> -->
+ <!-- </script> -->
+ <!-- </material> -->
can you remove the commented out lines?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2811 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADYNXFKYZBER65TUSJBI4LSACZRLANCNFSM4PUJJ54A>
.
|
iche033
approved these changes
Aug 11, 2020
|
@iche033 thanks for merge, BTW is it too late to apply this patch to gazebo9 and use them with melodic? Or if I write PR against |
|
@k-okada sure you can create a PR backporting this fix to the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
e070098 (Kei Okada, 61 seconds ago)
Length of polylist
<p>is not the number of inputs (i.e. VERTEX and NORMAL) * vcount.size() * 3. If both VERTEX and NORMAL have offset 0, then the length of polylist<p>is vcount.size() * 3. So use length of set(offset) as inputSize.This fix for cases where VERTEX and NROMAL share same vcounts, see Segmentation fault on spawning model with collada mesh generated by assimp on ubuntu 18.04 #2682 (comment)
b344fd9 (Kei Okada, 21 minutes ago)
add test to check gazebo model generated by assimp4, see Segmentation fault on spawning model with collada mesh generated by assimp on ubuntu 18.04 #2682