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 10k actors and generalize actors pose in erb #336

Merged
merged 6 commits into from
Oct 6, 2020

Conversation

claireyywang
Copy link
Contributor

minor tweaks to add more descriptive actors and shapes sdf file names, and generalize actors pose in erb file

Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang claireyywang self-assigned this Sep 3, 2020
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Sep 3, 2020
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Like was mentioned during the meeting, let's not commit the large generated files because they can be misleading to users, and also significantly increase the size of the source code and install packages.

Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang claireyywang requested a review from chapulina October 5, 2020 23:13
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Works well! I just have a suggestion to remove the lonely guy who doesn't follow the crowd 😅

@@ -138,13 +142,26 @@
</include>

<%
# number of population
Copy link
Contributor

Choose a reason for hiding this comment

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

The dude on line 141 seems a bit lost in the crowd, is he needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean this # number of population line? I think you are looking at the old file? This is the new change equivalent to that line https://github.com/ignitionrobotics/ign-gazebo/pull/336/files#diff-b1617932cd58db140bca53385d0da9bcR145

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry, I meant the included actor around line 141 of the new file:

    <include>
      <uri>https://fuel.ignitionrobotics.org/1.0/Mingfei/models/actor</uri>
    </include>

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember the lonely actor used to be able to walk around others without bumping into anyone :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooo that's what it was. I was wondering why there's always an extra actor different from others. Yeah I'll remove it.

Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
@claireyywang
Copy link
Contributor Author

Failed test cases on osx and ubuntu are not introduced by this PR. Merging...

@claireyywang claireyywang merged commit fdb71cc into ign-gazebo3 Oct 6, 2020
@claireyywang claireyywang deleted the claire/generalize_actor_population branch October 6, 2020 22:20
doisyg pushed a commit to wyca-robotics/ign-gazebo that referenced this pull request Dec 13, 2020
* add more descriptive sdf names and generalize actors erb

Signed-off-by: claireyywang <22240514+claireyywang@users.noreply.github.com>
Signed-off-by: Guillaume Doisy <guillaume.doisy@wyca.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants