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.

Sign up for GitHub

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

Jump to bottom

Support the case material's name is empty string #374

Merged
merged 1 commit into from
Sep 8, 2020
Merged

Support the case material's name is empty string #374

merged 1 commit into from
Sep 8, 2020

Conversation

buster84
Copy link
Contributor

@buster84 buster84 commented Sep 7, 2020

Some robot's definition has empty string for material's name. And the empty string caused this.materials mess up.
This fix the problem.

@gavanderhoorn
Copy link

Related: ros-industrial/fanuc#297.

@gavanderhoorn gavanderhoorn mentioned this pull request Sep 7, 2020
Closed
@Rayman
Copy link
Contributor

Rayman commented Sep 7, 2020

So are materials defined to be optional in urdf?

@gavanderhoorn
Copy link

So are materials defined to be optional in urdf?

The technical answer to that would be "yes", as you're not required to use any material tags in a URDF.

But I believe you're asking whether the name attribute on material elements is optional. If that's the case, then: no, it's not optional.

What we're doing in ros-industrial/fanuc though is using empty names for locally defined materials. That would be something else.

@buster84
Copy link
Contributor Author

buster84 commented Sep 7, 2020

I think what @gavanderhoorn said is correct.

The official c++ implementation also has the special handling for empty string. See here

In my opinion, I don't think there is a technical reason why name attribute on material elements is required because empty string is allowed. But it has been there for long time and it is hard to change the definition I guess.

@buster84
Copy link
Contributor Author

buster84 commented Sep 7, 2020

@Rayman I think CI is failing because of this issue ros/genpy#132 .
We need to add apt update && apt dist-upgrade in dockerfile. Is that okay to add in this PR or should I create another PR?

@Rayman
Copy link
Contributor

Rayman commented Sep 8, 2020

If I read ros/genpy#132 it makes sense to add apt-get upgrade to the dockerfile. You could try to fix the CI for this PR

I'm not a maintainer though, I just have a personal interest in this package.

@buster84
Copy link
Contributor Author

buster84 commented Sep 8, 2020 •
edited
Loading

If I read ros/genpy#132 it makes sense to add apt-get upgrade to the dockerfile. You could try to fix the CI for this PR

I'm not a maintainer though, I just have a personal interest in this package.

@Rayman But you can merge this PR and release new version, can you? anyway, I made CI passed so please approve this or let me know if I need to do something else.

@Rayman
Copy link
Contributor

Rayman commented Sep 8, 2020

I can't, you would have to ask @mvollrath

mvollrath
mvollrath requested changes Sep 8, 2020
View reviewed changes
Dockerfile Outdated
@@ -4,6 +4,9 @@ ENV ROS_DISTRO=kinetic
# Dependencies for rosbridge
RUN apt update && apt-get install -y xvfb firefox git wget ros-$ROS_DISTRO-rosbridge-server ros-$ROS_DISTRO-tf2-web-republisher ros-$ROS_DISTRO-common-tutorials ros-$ROS_DISTRO-rospy-tutorials ros-$ROS_DISTRO-actionlib-tutorials

# Because of this issue https://github.com/ros/genpy/issues/132
RUN apt update && apt-get upgrade -y
Copy link
Contributor

@mvollrath mvollrath Sep 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Doing this tends to cost a lot of time and space, and makes images less consistent. The base image should be updated soon. I appreciate that this is needed for the tests to pass, I would merge it without this change.

@buster84
Copy link
Contributor Author

buster84 commented Sep 8, 2020

@mvollrath thank you for the reviewing. I understood and took away the commit.

@buster84
Copy link
Contributor Author

buster84 commented Sep 8, 2020

BTW, #359 is also the same problem and I hope this fix solves the problem.

@mvollrath mvollrath merged commit b39bf03 into RobotWebTools:develop Sep 8, 2020
@Rayman Rayman mentioned this pull request Sep 9, 2020
Closed
MatthijsBurgh pushed a commit that referenced this pull request Jun 20, 2021
@buster84 @MatthijsBurgh
Support the case material's name is empty string (#374)
8e7095f
k-aguete pushed a commit to k-aguete/roslibjs that referenced this pull request Oct 21, 2022
@dependabot
Bump debug from 3.2.7 to 4.3.1 (RobotWebTools#374)
531dcc5 
Bumps [debug](https://github.com/visionmedia/debug) from 3.2.7 to 4.3.1.
- [Release notes](https://github.com/visionmedia/debug/releases)
- [Commits](debug-js/debug@3.2.7...4.3.1)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rayman Rayman Rayman approved these changes

@mvollrath mvollrath mvollrath approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@buster84 @gavanderhoorn @Rayman @mvollrath