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

Adds python interface to MaterialType and Material. #234

Merged
merged 3 commits into from
Oct 7, 2021

Conversation

LolaSegura
Copy link
Contributor

🎉 New feature

Related to #101 #210

Summary

Adds Python interface for three math classes: MaterialType, Material. For each Material a python test has been created.

Related issues and notes

MaterialType

This file contains an enum class declaration. Swig supports strongly typed enumerations, so binding this wasn't a problem. But the result was a series of variables contained in the module ignition.math defined as MaterialType_MATERIAL. A work around is needed to be implemented in order to follow the pep-8 naming convention.

Material

The test is not passing.

  • This class contains a method Predefined() that returns a reference to an std::map<MaterialType, Material>. In SignalStatsa map was used as well, so a similar approach was taken for this case. There are two main differences in both of this classes:

    • In SignalStats the map is returned as a copy and
    • the types of the map are std::string and double.
  • In the case of Material the method is not being wrapped correctly. Instead of being part of the Material class, is defined outside of it, and when it is used in the test as follow:

    mats = ignition.math.Material.predefined() 
    

    it arises an error:

    swig/python detected a memory leak of type 'unknown', no destructor found.
       TypeError: unhashable type: 'SwigPyObject'
       The above exception was the direct cause of the following exception:
    
        Traceback (most recent call last):
         File "/ws/src/ign-math/src/python/Material_TEST.py", line 22, in test_init
            mats = ignition.math.Material.predefined()
        SystemError: <built-in function Material_predefined> returned a result with an error set 
    
    
    

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

Signed-off-by: LolaSegura <lsegura@ekumenlabs.com>
@github-actions github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress 🏰 citadel Ignition Citadel 🔮 dome Ignition Dome labels Aug 31, 2021
@osrf-triage osrf-triage added this to Inbox in Core development Aug 31, 2021
@chapulina chapulina moved this from Inbox to In progress in Core development Sep 1, 2021
@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2021
@chapulina chapulina removed the beta Targeting beta release of upcoming collection label Sep 20, 2021
@ahcorde ahcorde mentioned this pull request Sep 23, 2021
7 tasks
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde marked this pull request as ready for review October 6, 2021 18:41
@ahcorde ahcorde requested a review from scpeters as a code owner October 6, 2021 18:41
@codecov
Copy link

codecov bot commented Oct 6, 2021

Codecov Report

Merging #234 (0e5fb4a) into ign-math6 (23a24d7) will not change coverage.
The diff coverage is n/a.

❗ Current head 0e5fb4a differs from pull request most recent head e69094b. Consider uploading reports for the commit e69094b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           ign-math6     #234   +/-   ##
==========================================
  Coverage      99.41%   99.41%           
==========================================
  Files             67       67           
  Lines           6347     6347           
==========================================
  Hits            6310     6310           
  Misses            37       37           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23a24d7...e69094b. Read the comment docs.

@ahcorde
Copy link
Contributor

ahcorde commented Oct 6, 2021

friendly ping @scpeters

@ahcorde ahcorde force-pushed the LolaSegura/add_material-type_material branch from 53a6420 to e69094b Compare October 7, 2021 07:41
Core development automation moved this from In progress to In review Oct 7, 2021
@scpeters scpeters merged commit f93eeea into ign-math6 Oct 7, 2021
@scpeters scpeters deleted the LolaSegura/add_material-type_material branch October 7, 2021 18:01
Core development automation moved this from In review to Done Oct 7, 2021
@francocipollone
Copy link
Contributor

Thanks @ahcorde for taking over the PR and completing it! 🚀

@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel 🔮 dome Ignition Dome 🏢 edifice Ignition Edifice 🏯 fortress Ignition Fortress Gazebo 1️1️ Dependency of Gazebo classic version 11
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants