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

Names with spaces: add string serializer #244

Merged
merged 4 commits into from
Jul 20, 2020

Conversation

chapulina
Copy link
Contributor

Addresses issue #239 .

This needs to target Dome because changing the serializer from default to a custom string serializer breaks ABI.

Added some names with spaces and special characters to fuel.sdf and everything seems to be in place:

names_spaces

@github-actions github-actions bot added the 🔮 dome Ignition Dome label Jul 15, 2020
@codecov
Copy link

codecov bot commented Jul 15, 2020

Codecov Report

Merging #244 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
+ Coverage   65.78%   65.82%   +0.03%     
==========================================
  Files         128      128              
  Lines        6307     6314       +7     
==========================================
+ Hits         4149     4156       +7     
  Misses       2158     2158              
Impacted Files Coverage Δ
include/ignition/gazebo/components/Actor.hh 27.27% <ø> (ø)
...nclude/ignition/gazebo/components/ChildLinkName.hh 100.00% <ø> (ø)
include/ignition/gazebo/components/Name.hh 100.00% <ø> (ø)
...clude/ignition/gazebo/components/ParentLinkName.hh 100.00% <ø> (ø)
...de/ignition/gazebo/components/PerformerAffinity.hh 100.00% <ø> (ø)
.../ignition/gazebo/components/PhysicsEnginePlugin.hh 100.00% <ø> (ø)
...clude/ignition/gazebo/components/SourceFilePath.hh 100.00% <ø> (ø)
...ude/ignition/gazebo/components/LevelEntityNames.hh 100.00% <100.00%> (ø)
...nclude/ignition/gazebo/components/Serialization.hh 100.00% <100.00%> (ø)

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 3518572...08f8019. Read the comment docs.

@luca-della-vedova
Copy link
Member

luca-della-vedova commented Jul 15, 2020

It did fix the errors being printed, however multiple models with spaces are still rendered incorrectly, i.e. the sdf snippet wrote in the issue renders as below:

image

Edit: Actually nevermind I believe it is just an issue with the asset and its use of material scripts for alpha textures that is not supported anymore in ignition

Copy link
Member

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for iterating so quickly on this!
I guess as usual add the Changelog?

@chapulina
Copy link
Contributor Author

@osrf-jenkins retest this please

@chapulina
Copy link
Contributor Author

  • Ubuntu actions CI has never passed.
  • Ubuntu Jenkins CI is currently broken for Dome.
  • Homebrew doesn't have any new failures.

@chapulina chapulina merged commit aea1acc into master Jul 20, 2020
@chapulina chapulina deleted the chapulina/string_serializer branch July 20, 2020 19:02
j-rivero pushed a commit that referenced this pull request Jul 29, 2020
Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
j-rivero added a commit that referenced this pull request Aug 3, 2020
* Names with spaces: add string serializer (#244)
* Update to Dome versions
* Update dependencies in workflows
* Add X support to workflows

Signed-off-by: Louise Poubel <[email protected]>
Signed-off-by: Jose Luis Rivero <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
Co-authored-by: Nate Koenig <[email protected]>
Co-authored-by: Jose Luis Rivero <[email protected]>
@chapulina chapulina mentioned this pull request Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants