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

Add transformation matrix to ColladaExport #100

Merged
merged 13 commits into from
Nov 18, 2020

Conversation

gonzodepedro
Copy link
Contributor

@gonzodepedro gonzodepedro commented Sep 28, 2020

Signed-off-by: Gonzalo de Pedro [email protected]

This adds transformation matrixes to nodes con collada files when exporting

Connects to gazebosim/gz-sim#307

@nkoenig nkoenig requested a review from iche033 September 28, 2020 20:17
@chapulina chapulina added 🏰 citadel Ignition Citadel 📜 blueprint Ignition Blueprint 🔮 dome Ignition Dome labels Sep 28, 2020
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

I was able to export a double pendulum model so I think simple shapes are working. Can you add a test in ColladaExporter_TEST? Something like exporting multiple meshes to a dae file then loading it back with ColladaLoader to make sure they are loaded back as submeshes?

graphics/src/ColladaExporter.cc Outdated Show resolved Hide resolved
graphics/include/ignition/common/ColladaExporter.hh Outdated Show resolved Hide resolved
graphics/src/ColladaExporter.cc Outdated Show resolved Hide resolved
@mjcarroll
Copy link
Contributor

@osrf-jenkins run tests

Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

Prefer common::joinPaths if you can.

Comment on lines +200 to +201
std::string boxFilenameIn = std::string(PROJECT_SOURCE_PATH) +
"/test/data/box.dae";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::string boxFilenameIn = std::string(PROJECT_SOURCE_PATH) +
"/test/data/box.dae";
std::string boxFilenameIn = common::joinPaths(PROJECT_SOURCE_PATH,
"/test/data/box.dae");

Copy link
Contributor

Choose a reason for hiding this comment

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

Still needed.

Copy link
Contributor

@chapulina chapulina Dec 3, 2020

Choose a reason for hiding this comment

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

It looks like this went through unresolved partially resolved. The string needs to be separated on all / for Windows.

I also noticed this test failing on CI:

  [ RUN      ] ColladaExporter.ExportCordlessDrill
  unknown file: Failure
  C++ exception with description "basic_string::_M_construct null not valid" thrown in the test body.
  [  FAILED  ] ColladaExporter.ExportCordlessDrill (3 ms)
  [ RUN      ] ColladaExporter.ExportMeshWithSubmeshes
  unknown file: Failure
  C++ exception with description "basic_string::_M_construct null not valid" thrown in the test body.
  [  FAILED  ] ColladaExporter.ExportMeshWithSubmeshes (3 ms)

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this didn't fail in the action here?

Fix here: #133

graphics/src/ColladaExporter_TEST.cc Outdated Show resolved Hide resolved
graphics/src/ColladaExporter_TEST.cc Outdated Show resolved Hide resolved
graphics/src/ColladaExporter_TEST.cc Outdated Show resolved Hide resolved
graphics/src/ColladaExporter.cc Show resolved Hide resolved
graphics/src/ColladaExporter.cc Outdated Show resolved Hide resolved
graphics/src/ColladaExporter.cc Show resolved Hide resolved
Gonzalo de Pedro added 10 commits November 9, 2020 18:01
Signed-off-by: Gonzalo de Pedro <[email protected]>
Signed-off-by: Gonzalo de Pedro <[email protected]>
Signed-off-by: Gonzalo de Pedro <[email protected]>
Signed-off-by: Gonzalo de Pedro <[email protected]>
Signed-off-by: Gonzalo de Pedro <[email protected]>
Signed-off-by: Gonzalo de Pedro <[email protected]>
Signed-off-by: Gonzalo de Pedro <[email protected]>
Signed-off-by: Gonzalo de Pedro <[email protected]>
Signed-off-by: Gonzalo de Pedro <[email protected]>
Signed-off-by: Gonzalo de Pedro <[email protected]>
Gonzalo de Pedro added 2 commits November 9, 2020 18:11
Signed-off-by: Gonzalo de Pedro <[email protected]>
Signed-off-by: Gonzalo de Pedro <[email protected]>
Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

A few lingering unresolved bits

@gonzodepedro
Copy link
Contributor Author

A few lingering unresolved bits

Copy link
Contributor

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for iterating.

@mjcarroll mjcarroll merged commit c2f86eb into gazebosim:ign-common3 Nov 18, 2020
mjcarroll added a commit that referenced this pull request Dec 3, 2020
Follow up fix to #100

Signed-off-by: Michael Carroll <[email protected]>
mjcarroll added a commit that referenced this pull request Dec 3, 2020
Follow up fix to #100

Signed-off-by: Michael Carroll <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📜 blueprint Ignition Blueprint 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants